Merge lp:~jamesodhunt/upstart/bug-530779 into lp:upstart

@@ -275,7 +280,18 @@
        if (! state_check_json_type (json_processes, array))
                goto error;

- for (i = 0; i < json_object_array_length (json_processes); i++) {
+ len = json_object_array_length (json_processes);
+
+ if (len > PROCESS_LAST) {
+ /* Detected a downgrade on re-exec scenario (where we
+ * are attempting to restore state from a newer init
+ * which contains additional processes
+ */
+ nih_warn ("%s", _("Detected unsupported downgrade on re-exec"));
+ goto error;
+ }
+
+ for (i = 0; i < len; i++) {
                json_object *json_process;

                nih_assert (i <= PROCESS_LAST);

The result of this is that, instead of deserializing those processes that are known, on any downgrade to a version that tracked fewer processes, all information will be discarded about all processes associated with jobs. This is effectively equivalent to a stateless reexec, and is not a very graceful handling of this case.

I don't feel very strongly in general about supporting downgrades; but in a case such as this where you need to explicitly handle the difference in the number of related processes, it seems to me that a graceful handling is as easy to accomplish as the ungraceful alternative.

+ TEST_FEATURE ("full double-fork daemon test where parent waits for ultimate child");

does not fit the expected structure of a test feature name. Suggest instead:
        TEST_FEATURE ("with daemon where parent waits for ultimate child before exiting");

+ assert0 (prctl (PR_SET_CHILD_SUBREAPER, 1));

Upstart does not depend on PR_SET_CHILD_SUBREAPER being available on the running kernel - the test suite shouldn't either. The test needs to be skipped if this facility is unavailable, or it needs to be rewritten to avoid subreaper entirely.

+ /* Low byte is signal */
+ TEST_EQ (siginfo.si_status, SIGTRAP);
+
+ /* Normally, the next byte would be the ptrace event,
+ * but upstart hasn't yet set PTRACE_O_TRACEEXEC, hence
+ * no ptrace event will be available.
+ */
+ TEST_EQ (((siginfo.si_status & ~0x7f)>>8), 0);

The second test here is redundant with the first; if siginfo.si_status == SIGTRAP, the high byte must be 0. Either the second test should be dropped, or, if you want to keep it for clarity, the first test should be modified to check siginfo.si_status & 0x7f.

+ TIMED_BLOCK (5) {

I'm not thrilled about this TIMED_BLOCK() idea. Why should we not just use inotify? For that matter, why is this pidfile needed at all - what is this meant to guard against? The test case already includes its own direct check of the pids with ptrace, so the only thing this pidfile does is make sure the daemon agrees with the test what the PIDs are. That seems unnecessary to me.

+ /* Wait for child to send itself SIGSTOP denoting its
+ * final resting state.
+ */
+ got = FALSE;
+ TIMED_BLOCK (5) {
+
+ memset (&siginfo, 0, sizeof (siginfo));
+ ret = waitid (P_PID, final_pid, &siginfo, WSTOPPED | WNOWAIT | WUNTRACED);
+
+ if (! ret && siginfo.si_code == CLD_STOPPED &&
+ siginfo.si_status == SIGSTOP) {
+ got = TRUE;
+ break;
+ }
+ }

This section seems overly complicated. The child process could just call pause() instead, and let the test runner kill the process when it's done.

+ TIMED_BLOCK (5) {
+ nih_child_poll ();
+
+ if (kill (final_pid, 0) == -1 && errno == ESRCH) {
+ got = TRUE;
+ break;
+ }
+ }

Definitely no reason for this to be in a TIMED_BLOCK(). Or called at all. We've already received CLD_EXITED above, so the kill() should immediately return ESRCH. But again, this code all goes away if we just kill SIGKILL the final_pid, instead of including these checks that are unrelated to the purpose of the test case.

 static void
 selfpipe_setup (void)
 {
- static struct sigaction act;
- int read_flags;
- int write_flags;
[...]

This mixes formatting cha...

Hi Steve,

I've now updated the deserialisation to make an attempt on the downgrade scenario. Note the comment in process.h though regarding the ordering of ProcessType. I've also raised bug 1256985 whilst looking at this.

> + TEST_FEATURE ("full double-fork daemon test where parent waits for ultimate child");
>
> does not fit the expected structure of a test feature name. Suggest instead:
> TEST_FEATURE ("with daemon where parent waits for ultimate child before exiting");

Fixed.

>
> + assert0 (prctl (PR_SET_CHILD_SUBREAPER, 1));
>
> Upstart does not depend on PR_SET_CHILD_SUBREAPER being available on the running kernel - the test suite shouldn't either. The test needs to be skipped if this facility is unavailable, or it needs to be rewritten to avoid subreaper entirely.

Fixed.

>
> + /* Low byte is signal */
> + TEST_EQ (siginfo.si_status, SIGTRAP);
> +
> + /* Normally, the next byte would be the ptrace event,
> + * but upstart hasn't yet set PTRACE_O_TRACEEXEC, hence
> + * no ptrace event will be available.
> + */
> + TEST_EQ (((siginfo.si_status & ~0x7f)>>8), 0);
>
> The second test here is redundant with the first; if siginfo.si_status == SIGTRAP, the high byte must be 0. Either the second test should be dropped, or, if you want to keep it for clarity, the first test should be modified to check siginfo.si_status & 0x7f.

Fixed.

>
> + TIMED_BLOCK (5) {
>
> I'm not thrilled about this TIMED_BLOCK() idea. Why should we not just use inotify?

To give some context here, it's worth pointing out that this test has been through a few iterations. An earlier incarnation of it was using signals entirely but that is problematic since:

  - test_daemon needed to raise SIGSTOP on itself.
  - test_daemon is being ptraced hence is also getting SIGSTOP's sent to it by the kernel.
  - This is a multi-process test so we need to guarantee behaviour given that we can't know the order the kernel will schedule each process.
  - We don't want to call nih_main_loop() as we cannot then control the test in "single-step" increments (well, as close as we can get to that anyway :-).

Certainly inotify would be the obvious choice but for that fact that it would add further code and complexity to the test since we're avoiding using nih_main_loop().

TIMED_BLOCK offers a rather useful generic facility I think, particularly since a common requirement for the tests is to perform some operation "within a reasonable amount of time" and then fail. Yes, it's tricky to determine the value of "reasonable", but we do need to guarantee a hard test failure, rather than an indefinite hang.

Admittedly, using TIMED_BLOCK() with file_line_count() is rather grisly, but even if we were to use inotify, we'd still need to check the line count on each event as we need to know when a particular parent is in a particular state.

> For that matter, why is this pidfile needed at all - what is this meant to guard against? The test case already includes its own direct check of the pids with ptrace, so the only thing this pidfile does is make sure the daemon agrees with the test what the PIDs are. That seems unnecessary to me.

It's not immediately obvious, but that's not th...

Hi Steve - I've now cleaned up the branch.

>> Upstart does not depend on PR_SET_CHILD_SUBREAPER being
>> available on the running kernel - the test suite shouldn't
>> either. The test needs to be skipped if this facility is
>> unavailable, or it needs to be rewritten to avoid subreaper
>> entirely.

> Fixed.

This is not fixed. You've adjusted the test suite to not fail if PR_SET_CHILD_SUBREAPER is not *defined* at build time; this does not address the problem of it being *unsupported* by the running kernel. The 'assert0 (prctl (PR_SET_CHILD_SUBREAPER, 1))' must be dropped from the test suite, and be replaced by a check that skips the test if it's unavailable.

This probably calls for the test to be broken out into a separate function so that it can be skipped sensibly.

Hi Steve,

Thanks for reviewing - code updated.