posix/tst-spawn: Fix racy tests in spawned processes.
Commit Message
On 28/01/2019 11:41, Stefan Liebler wrote:
> Hi,
>
> from time to time I get fails in tst-spawn like:
> tst-spawn.c:111: numeric comparison failure
> left: 0 (0x0); from: xlseek (fd2, 0, SEEK_CUR)
> right: 28 (0x1c); from: strlen (fd2string)
> error: 1 test failures
> tst-spawn.c:252: numeric comparison failure
> left: 1 (0x1); from: WEXITSTATUS (status)
> right: 0 (0x0); from: 0
> error: 1 test failures
>
> It turned out, that a child process is testing it's open file descriptors with e.g. a sequence of testing the current position, setting the position to zero and reading a specific amount of bytes.
>
> Unfortunately starting with commit 2a69f853c03034c2e383e0f9c35b5402ce8b5473 the test is spawning a second child process which is sharing some of the file descriptors. If the test sequence as mentioned above is running in parallel it leads to test failures.
>
> As the second call of posix_spawn shall test a NULL pid argument, this patch is just moving the waitpid of the first child before the posix_spawn of the second child.
>
> Okay for commit?
> (I assume 2.30)
>
> Bye,
> Stefan
>
> ChangeLog:
>
> * posix/tst-spawn do_test(): Move waitpid before posix_spawn.
>
It looks you forgot to add the patch itself on the message, but based on the description
I think you meant something like the below:
right?
Comments
On 01/28/2019 03:15 PM, Adhemerval Zanella wrote:
>
>
> On 28/01/2019 11:41, Stefan Liebler wrote:
>> Hi,
>>
>> from time to time I get fails in tst-spawn like:
>> tst-spawn.c:111: numeric comparison failure
>> left: 0 (0x0); from: xlseek (fd2, 0, SEEK_CUR)
>> right: 28 (0x1c); from: strlen (fd2string)
>> error: 1 test failures
>> tst-spawn.c:252: numeric comparison failure
>> left: 1 (0x1); from: WEXITSTATUS (status)
>> right: 0 (0x0); from: 0
>> error: 1 test failures
>>
>> It turned out, that a child process is testing it's open file descriptors with e.g. a sequence of testing the current position, setting the position to zero and reading a specific amount of bytes.
>>
>> Unfortunately starting with commit 2a69f853c03034c2e383e0f9c35b5402ce8b5473 the test is spawning a second child process which is sharing some of the file descriptors. If the test sequence as mentioned above is running in parallel it leads to test failures.
>>
>> As the second call of posix_spawn shall test a NULL pid argument, this patch is just moving the waitpid of the first child before the posix_spawn of the second child.
>>
>> Okay for commit?
>> (I assume 2.30)
>>
>> Bye,
>> Stefan
>>
>> ChangeLog:
>>
>> * posix/tst-spawn do_test(): Move waitpid before posix_spawn.
>>
>
> It looks you forgot to add the patch itself on the message, but based on the description
> I think you meant something like the below:
>
> diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
> index eea5add..2aa0dcc 100644
> --- a/posix/tst-spawn.c
> +++ b/posix/tst-spawn.c
> @@ -237,25 +237,25 @@ do_test (int argc, char *argv[])
> TEST_COMPARE (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ),
> 0);
>
> - /* Same test but with a NULL pid argument. */
> - TEST_COMPARE (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ),
> - 0);
> -
> - /* Cleanup. */
> - TEST_COMPARE (posix_spawn_file_actions_destroy (&actions), 0);
> - free (name3_copy);
> -
> /* Wait for the children. */
> TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
> TEST_VERIFY (WIFEXITED (status));
> TEST_VERIFY (!WIFSIGNALED (status));
> TEST_COMPARE (WEXITSTATUS (status), 0);
>
> + /* Same test but with a NULL pid argument. */
> + TEST_COMPARE (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ),
> + 0);
> +
> xwaitpid (-1, &status, 0);
> TEST_VERIFY (WIFEXITED (status));
> TEST_VERIFY (!WIFSIGNALED (status));
> TEST_COMPARE (WEXITSTATUS (status), 0);
>
> + /* Cleanup. */
> + TEST_COMPARE (posix_spawn_file_actions_destroy (&actions), 0);
> + free (name3_copy);
> +
> return 0;
> }
>
> right?
>
Yes, this is similar except that the "cleanup" is done before the
"waitpid(-1)" as before.
See my mail in this thread with the "attached" patch.
@@ -237,25 +237,25 @@ do_test (int argc, char *argv[])
TEST_COMPARE (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ),
0);
- /* Same test but with a NULL pid argument. */
- TEST_COMPARE (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ),
- 0);
-
- /* Cleanup. */
- TEST_COMPARE (posix_spawn_file_actions_destroy (&actions), 0);
- free (name3_copy);
-
/* Wait for the children. */
TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
TEST_VERIFY (WIFEXITED (status));
TEST_VERIFY (!WIFSIGNALED (status));
TEST_COMPARE (WEXITSTATUS (status), 0);
+ /* Same test but with a NULL pid argument. */
+ TEST_COMPARE (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ),
+ 0);
+
xwaitpid (-1, &status, 0);
TEST_VERIFY (WIFEXITED (status));
TEST_VERIFY (!WIFSIGNALED (status));
TEST_COMPARE (WEXITSTATUS (status), 0);
+ /* Cleanup. */
+ TEST_COMPARE (posix_spawn_file_actions_destroy (&actions), 0);
+ free (name3_copy);
+
return 0;
}