[Merge] lp:~upstart-devel/upstart/stateful-reexec into lp:upstart
James Hunt
james.hunt at canonical.com
Wed Nov 14 14:54:41 UTC 2012
> Hi James,
>
> Reviewing just the parts of the diff that are new since the last time I looked at the branch.
>
> This part of init/session.c looks strange:
>
> + if (getenv ("UPSTART_TESTS"))
> + return;
> +
> if (conf_source_reload (source) < 0) {
> NihError *err;
>
> The commit message only says:
> - session_create_conf_source(): Don't reload conf sources when in
> testing mode.
> without explaining why.
I've now removed this: it was required for an earlier iteration of the test code, but is now redundant.
>
> In init/tests/test_util.c:
>
> + * Returns 0 if strings are identical or both NULL, else 1.
> + **/
> +int
> +string_check (const char *a, const char *b)
> +{
> + if ((a == b) && !a)
> + return 0;
> +
>
> This '&& !a' seems unnecessary - surely if a == b and are /not/ null, there's no need to call strcmp to confirm that the strings are equal.
That test is a just a more compact way of saying:
if (a == NULL && b == NULL)
... such that -- considered with the following test -- we never pass NULL to strcmp(). However, it is slightly cryptic, so for symmetry with the test that follows it I've changed the test to:
if (!a && !b)
>
>
> === modified file 'init/blocked.c'
> --- init/blocked.c 2012-07-30 14:07:37 +0000
> +++ init/blocked.c 2012-11-13 06:01:51 +0000
> @@ -131,6 +131,9 @@ blocked_type_enum_to_str (BlockedType ty
> BlockedType
> blocked_type_str_to_enum (const char *type)
> {
> + if (! type)
> + goto error;
> +
> state_str_to_enum (BLOCKED_JOB, type);
> state_str_to_enum (BLOCKED_EVENT, type);
> state_str_to_enum (BLOCKED_EMIT_METHOD, type);
> @@ -141,5 +144,6 @@ blocked_type_str_to_enum (const char *ty
> state_str_to_enum (BLOCKED_INSTANCE_STOP_METHOD, type);
> state_str_to_enum (BLOCKED_INSTANCE_RESTART_METHOD, type);
>
> +error:
> return -1;
> }
>
>
> The above is documented as "Made resilient to error conditions". But this doesn't appear to be a real-world error condition at all; the only place in the code that blocked_type_str_to_enum() is called is in state.c:
>
> if (! state_get_json_string_var_strict (json, "type", NULL, blocked_type_str))
> goto error;
>
> blocked_type = blocked_type_str_to_enum (blocked_type_str);
>
> This means there's no possibility of the function ever being called with a null argument (and likewise for the other str_to_enum functions), so this must be here just for the test suite. I don't think it's a good idea to complicate either the code or the test suite by handling errors that the code is written to never allow! I've pushed a branch to address this at
> lp:~vorlon/upstart/stateful-reexec-dont-shape-code-to-impossible-tests and raised an MP for your consideration.
Thanks. I've changed this and the other *_str_to_enum() functions to assert their argument and updated the tests accordingly.
>
> === modified file 'init/job_process.c'
> --- init/job_process.c 2012-08-25 08:53:37 +0000
> +++ init/job_process.c 2012-11-13 10:47:50 +0000
> @@ -211,7 +211,7 @@ job_process_run (Job *job,
> while (p && (*p == '\n'))
> p++;
>
> - if ((! nl) || (! *p)) {
> + if ((! nl) || (p && ! *p)) {
> /* Strip off the newline(s) */
> if (nl)
> *nl = '\0';
>
> This change is documented as "Stop potential NULL-pointer dereference". But here's the full context:
>
> p = nl = strchr (script, '\n');
> while (p && (*p == '\n'))
> p++;
>
> if ((! nl) || (p && ! *p)) {
> /* Strip off the newline(s) */
> if (nl)
> *nl = '\0';
>
> So the only way p can be NULL is if 'script' contains no newlines. In that case, nl is also null, and the first part of the if will short-circuit without ever trying to dereference p. Why did you consider this a potential NULL dereference? Did this come from a compiler warning?
I'd already picked over this, but had overlooked that it hadn't been removed from this branch - reverted.
>
> init/log.c, log_deserialise():
>
> - /* Stop fd leaking to children */
> - if (log->fd != -1) {
> - if (state_toggle_cloexec (log->fd, TRUE) < 0)
> - goto error;
> - }
> + /* re-apply CLOEXEC flag to stop log file fd being leaked to children */
> + if (log->fd != -1 && state_toggle_cloexec (log->fd, TRUE) < 0)
> + return NULL;
>
> Neither of these is quite right. By this point, we've already successfully deserialized the log object. Returning NULL if we fail to set CLOEXEC doesn't help us - in fact, it means upstart will lose all track of the fd and will *never* close it, even when the associated job stops. So we should just ignore failures to set CLOEXEC on deserialization, so that we at least have a chance to clean up the fd later. Fix for this pushed to the branch.
We haven't actually deserialised the log at this point (since we haven't called log_new()). However, I agree that we would leak the fd in the failure scenario so your fix looks good.
>
> --- init/session.c 2012-09-24 14:43:33 +0000
> +++ init/session.c 2012-10-10 13:24:51 +0000
> @@ -467,9 +467,9 @@
> static Session *
> session_deserialise (json_object *json)
> {
> - Session *session;
> + Session *session = NULL;
> nih_local const char *chroot = NULL;
> - uid_t user;
> + uid_t user = (uid_t)-1;
>
> nih_assert (json);
>
> This is a misleading initialization. -1 is a perfectly valid value for a uid_t; it MUST NOT be used as an error sentinel. Fortunately nothing in the code below uses it this way, but it would be better to avoid such confusion.
Good catch - this was I'm sure added (erroneously) to appease a compiler warning at some stage. I'd like to see a system with that many users though ;-) Fixed.
>
> + * _state_get_json_str_array_generic:
> + *
> + * @parent: parent object for new array (may be NULL),
> + * @json: json_object pointer,
> + * @array: array to hold output,
> + * @len: length of @array,
> + * @env: TRUE if @json represents an array of environment variables,
> + * else FALSE,
>
> The 'env' parameter to this macro is completely unused. Not sure what the difference is meant to be between an env array and a regular string array?
>
> Otherwise this looks good to me. I think it's ready to land, with or without <lp:~vorlon/upstart/stateful-reexec-dont-shape-code-to-impossible-tests>.
Well spotted. An env array differs from a string array in that the former is an array of environment variables where values are expanded under certain circumstances.
--
https://code.launchpad.net/~upstart-devel/upstart/stateful-reexec/+merge/133275
Your team Upstart Reviewers is subscribed to branch lp:upstart.
More information about the upstart-devel
mailing list