Fix BZ#20544 (assert function passed to atexit/on_exit/__cxa_atexit != NULL)
Commit Message
* Paul Pluzhnikov:
> On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>
> Thanks for the review.
>
>> #else
>> {
>> struct support_capture_subprocess result;
>> result = support_capture_subprocess (do_test_bz20544_atexit, NULL);
>> support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT,
>> sc_allow_stderr);
>
> This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134.
>
> I notice that in libio/tst-vtables-common.c, Florian first initialized
> expected SIGABRT termination_status in init_termination_status(), and
> then used that in support_capture_subprocess_check() calls. Do I need
> to do the same here?
You can try the patch posted below. The initial API design was a bit
lazy.
> A different way to ask: do different OSes encode WIFSIGNALED /
> WIFEXITED differently?
Yes, but all glibc targets currently behave the same. I don't expect
this to change because the a lot of things are hard-coded (see the
JRuby/JVM discussion on reporting signal termination).
We should probably add macros to go into the other direction to
<sys/wait.h>, given that the layout is unlikely to change, ever.
Thanks,
Florian
-----
support: Add signal support to support_capture_subprocess_check
Adjust libio/tst-vtables-common.c to use this new functionality,
instead of determining the termination status for a signal indirectly.
2018-11-28 Florian Weimer <fweimer@redhat.com>
support: Add signal support to support_capture_subprocess_check.
* support/capture_subprocess.h (support_capture_subprocess_check):
Adjust comment and rename parameter.
* support/support_capture_subprocess_check.c
(print_actual_status): New function.
(support_capture_subprocess_check): Support negative
status_or_signal. Call print_actual_status.
* support/tst-support_capture_subprocess.c (do_test): Call
support_capture_subprocess_check.
* libio/tst-vtables-common.c (termination_status)
(init_termination_status): Remove.
(check_for_termination): Adjust support_capture_subprocess_check
call.
(do_test): Remove call to init_termination_status.
Comments
On Wed, Nov 28, 2018 at 11:07 AM Florian Weimer <fweimer@redhat.com> wrote:
> You can try the patch posted below. The initial API design was a bit
> lazy.
Looks good to me. I'll update my patch once you commit this one.
Thanks!
On 28/11/2018 17:07, Florian Weimer wrote:
> * Paul Pluzhnikov:
>
>> On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>
>> Thanks for the review.
>>
>>> #else
>>> {
>>> struct support_capture_subprocess result;
>>> result = support_capture_subprocess (do_test_bz20544_atexit, NULL);
>>> support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT,
>>> sc_allow_stderr);
>>
>> This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134.
>>
>> I notice that in libio/tst-vtables-common.c, Florian first initialized
>> expected SIGABRT termination_status in init_termination_status(), and
>> then used that in support_capture_subprocess_check() calls. Do I need
>> to do the same here?
>
> You can try the patch posted below. The initial API design was a bit
> lazy.
>
>> A different way to ask: do different OSes encode WIFSIGNALED /
>> WIFEXITED differently?
>
> Yes, but all glibc targets currently behave the same. I don't expect
> this to change because the a lot of things are hard-coded (see the
> JRuby/JVM discussion on reporting signal termination).
Do you have a link for the discussion?
>
> We should probably add macros to go into the other direction to
> <sys/wait.h>, given that the layout is unlikely to change, ever.
Is is something usually required and/or reimplemented elsewhere?
>
> Thanks,
> Florian
>
> -----
> support: Add signal support to support_capture_subprocess_check
>
> Adjust libio/tst-vtables-common.c to use this new functionality,
> instead of determining the termination status for a signal indirectly.
>
> 2018-11-28 Florian Weimer <fweimer@redhat.com>
>
> support: Add signal support to support_capture_subprocess_check.
> * support/capture_subprocess.h (support_capture_subprocess_check):
> Adjust comment and rename parameter.
> * support/support_capture_subprocess_check.c
> (print_actual_status): New function.
> (support_capture_subprocess_check): Support negative
> status_or_signal. Call print_actual_status.
> * support/tst-support_capture_subprocess.c (do_test): Call
> support_capture_subprocess_check.
> * libio/tst-vtables-common.c (termination_status)
> (init_termination_status): Remove.
> (check_for_termination): Adjust support_capture_subprocess_check
> call.
> (do_test): Remove call to init_termination_status.
LGTM, thanks.
>
> diff --git a/libio/tst-vtables-common.c b/libio/tst-vtables-common.c
> index 5e31012069..85e246cd11 100644
> --- a/libio/tst-vtables-common.c
> +++ b/libio/tst-vtables-common.c
> @@ -380,21 +380,6 @@ without_compatibility_fflush (void *closure)
> _exit (1);
> }
>
> -/* Exit status after abnormal termination. */
> -static int termination_status;
> -
> -static void
> -init_termination_status (void)
> -{
> - pid_t pid = xfork ();
> - if (pid == 0)
> - abort ();
> - xwaitpid (pid, &termination_status, 0);
> -
> - TEST_VERIFY (WIFSIGNALED (termination_status));
> - TEST_COMPARE (WTERMSIG (termination_status), SIGABRT);
> -}
> -
> static void
> check_for_termination (const char *name, void (*callback) (void *))
> {
> @@ -404,7 +389,7 @@ check_for_termination (const char *name, void (*callback) (void *))
> shared->calls = 0;
> struct support_capture_subprocess proc
> = support_capture_subprocess (callback, NULL);
> - support_capture_subprocess_check (&proc, name, termination_status,
> + support_capture_subprocess_check (&proc, name, -SIGABRT,
> sc_allow_stderr);
> const char *message
> = "Fatal error: glibc detected an invalid stdio handle\n";
> @@ -491,7 +476,6 @@ run_tests (bool initially_disabled)
>
> shared = support_shared_allocate (sizeof (*shared));
> shared->initially_disabled = initially_disabled;
> - init_termination_status ();
>
> if (initially_disabled)
> {
Ok.
> diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h
> index b0886ba1d1..4734b877ad 100644
> --- a/support/capture_subprocess.h
> +++ b/support/capture_subprocess.h
> @@ -49,13 +49,16 @@ enum support_capture_allow
> sc_allow_stderr = 0x04,
> };
>
> -/* Check that the subprocess exited with STATUS and that only the
> - allowed outputs happened. ALLOWED is a combination of
> - support_capture_allow flags. Report errors under the CONTEXT
> - message. */
> +/* Check that the subprocess exited and that only the allowed outputs
> + happened. If STATUS_OR_SIGNAL is nonnegative, it is the expected
> + (decoded) exit status of the process, as returned by WEXITSTATUS.
> + If STATUS_OR_SIGNAL is negative, -STATUS_OR_SIGNAL is the expected
> + termination signal, as returned by WTERMSIG. ALLOWED is a
> + combination of support_capture_allow flags. Report errors under
> + the CONTEXT message. */
> void support_capture_subprocess_check (struct support_capture_subprocess *,
> - const char *context, int status,
> - int allowed)
> + const char *context,
> + int status_or_signal, int allowed)
> __attribute__ ((nonnull (1, 2)));
>
> #endif /* SUPPORT_CAPTURE_SUBPROCESS_H */
Ok.
> diff --git a/support/support_capture_subprocess_check.c b/support/support_capture_subprocess_check.c
> index ff5ee89fb0..8b4c352c96 100644
> --- a/support/support_capture_subprocess_check.c
> +++ b/support/support_capture_subprocess_check.c
> @@ -20,6 +20,7 @@
> #include <stdio.h>
> #include <support/capture_subprocess.h>
> #include <support/check.h>
> +#include <sys/wait.h>
>
> static void
> print_context (const char *context, bool *failed)
> @@ -31,9 +32,22 @@ print_context (const char *context, bool *failed)
> printf ("error: subprocess failed: %s\n", context);
> }
>
> +static void
> +print_actual_status (struct support_capture_subprocess *proc)
> +{
> + if (WIFEXITED (proc->status))
> + printf ("error: actual exit status: %d [0x%x]\n",
> + WEXITSTATUS (proc->status), proc->status);
> + else if (WIFSIGNALED (proc->status))
> + printf ("error: actual termination signal: %d [0x%x]\n",
> + WTERMSIG (proc->status), proc->status);
> + else
> + printf ("error: actual undecoded exit status: [0x%x]\n", proc->status);
> +}
> +> void
> support_capture_subprocess_check (struct support_capture_subprocess *proc,
> - const char *context, int status,
> + const char *context, int status_or_signal,
> int allowed)
> {
> TEST_VERIFY ((allowed & sc_allow_none)
> @@ -44,11 +58,28 @@ support_capture_subprocess_check (struct support_capture_subprocess *proc,
> || (allowed & sc_allow_stderr))));
>
> bool failed = false;
> - if (proc->status != status)
> + if (status_or_signal >= 0)
> {
> - print_context (context, &failed);
> - printf ("error: expected exit status: %d\n", status);
> - printf ("error: actual exit status: %d\n", proc->status);
> + /* Expect regular termination. */
> + if (!(WIFEXITED (proc->status)
> + && WEXITSTATUS (proc->status) == status_or_signal))
> + {
> + print_context (context, &failed);
> + printf ("error: expected exit status: %d\n", status_or_signal);
> + print_actual_status (proc);
> + }
> + }
> + else
> + {
> + /* status_or_signal < 0. Expect termination by signal. */
> + if (!(WIFSIGNALED (proc->status)
> + && WTERMSIG (proc->status) == -status_or_signal))
> + {
> + print_context (context, &failed);
> + printf ("error: expected termination signal: %d\n",
> + -status_or_signal);
> + print_actual_status (proc);
> + }
> }
> if (!(allowed & sc_allow_stdout) && proc->out.length != 0)
> {
Ok.
> diff --git a/support/tst-support_capture_subprocess.c b/support/tst-support_capture_subprocess.c
> index a685256091..5339e85b07 100644
> --- a/support/tst-support_capture_subprocess.c
> +++ b/support/tst-support_capture_subprocess.c
> @@ -168,15 +168,29 @@ do_test (void)
> = support_capture_subprocess (callback, &test);
> check_stream ("stdout", &result.out, test.out);
> check_stream ("stderr", &result.err, test.err);
> +
> + /* Allowed output for support_capture_subprocess_check. */
> + int check_allow = 0;
> + if (lengths[length_idx_stdout] > 0)
> + check_allow |= sc_allow_stdout;
> + if (lengths[length_idx_stderr] > 0)
> + check_allow |= sc_allow_stderr;
> + if (check_allow == 0)
> + check_allow = sc_allow_none;
> +
> if (test.signal != 0)
> {
> TEST_VERIFY (WIFSIGNALED (result.status));
> TEST_VERIFY (WTERMSIG (result.status) == test.signal);
> + support_capture_subprocess_check (&result, "signal",
> + -SIGTERM, check_allow);
> }
> else
> {
> TEST_VERIFY (WIFEXITED (result.status));
> TEST_VERIFY (WEXITSTATUS (result.status) == test.status);
> + support_capture_subprocess_check (&result, "exit",
> + test.status, check_allow);
> }
> support_capture_subprocess_free (&result);
> free (test.out);
>
Ok.
* Adhemerval Zanella:
>> Yes, but all glibc targets currently behave the same. I don't expect
>> this to change because the a lot of things are hard-coded (see the
>> JRuby/JVM discussion on reporting signal termination).
>
> Do you have a link for the discussion?
I think this it:
<http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-June/028864.html>
In particular this:
<http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-June/028885.html>
Which mentions:
Shutdown.exit(sig.getNumber() + 0200);
>> We should probably add macros to go into the other direction to
>> <sys/wait.h>, given that the layout is unlikely to change, ever.
>
> Is is something usually required and/or reimplemented elsewhere?
I think it would be useful for writing tests at the very least.
Thanks,
Florian
* Paul Pluzhnikov:
> On Wed, Nov 28, 2018 at 11:07 AM Florian Weimer <fweimer@redhat.com> wrote:
>
>> You can try the patch posted below. The initial API design was a bit
>> lazy.
>
> Looks good to me. I'll update my patch once you commit this one.
Thanks, committed.
Florian
@@ -380,21 +380,6 @@ without_compatibility_fflush (void *closure)
_exit (1);
}
-/* Exit status after abnormal termination. */
-static int termination_status;
-
-static void
-init_termination_status (void)
-{
- pid_t pid = xfork ();
- if (pid == 0)
- abort ();
- xwaitpid (pid, &termination_status, 0);
-
- TEST_VERIFY (WIFSIGNALED (termination_status));
- TEST_COMPARE (WTERMSIG (termination_status), SIGABRT);
-}
-
static void
check_for_termination (const char *name, void (*callback) (void *))
{
@@ -404,7 +389,7 @@ check_for_termination (const char *name, void (*callback) (void *))
shared->calls = 0;
struct support_capture_subprocess proc
= support_capture_subprocess (callback, NULL);
- support_capture_subprocess_check (&proc, name, termination_status,
+ support_capture_subprocess_check (&proc, name, -SIGABRT,
sc_allow_stderr);
const char *message
= "Fatal error: glibc detected an invalid stdio handle\n";
@@ -491,7 +476,6 @@ run_tests (bool initially_disabled)
shared = support_shared_allocate (sizeof (*shared));
shared->initially_disabled = initially_disabled;
- init_termination_status ();
if (initially_disabled)
{
@@ -49,13 +49,16 @@ enum support_capture_allow
sc_allow_stderr = 0x04,
};
-/* Check that the subprocess exited with STATUS and that only the
- allowed outputs happened. ALLOWED is a combination of
- support_capture_allow flags. Report errors under the CONTEXT
- message. */
+/* Check that the subprocess exited and that only the allowed outputs
+ happened. If STATUS_OR_SIGNAL is nonnegative, it is the expected
+ (decoded) exit status of the process, as returned by WEXITSTATUS.
+ If STATUS_OR_SIGNAL is negative, -STATUS_OR_SIGNAL is the expected
+ termination signal, as returned by WTERMSIG. ALLOWED is a
+ combination of support_capture_allow flags. Report errors under
+ the CONTEXT message. */
void support_capture_subprocess_check (struct support_capture_subprocess *,
- const char *context, int status,
- int allowed)
+ const char *context,
+ int status_or_signal, int allowed)
__attribute__ ((nonnull (1, 2)));
#endif /* SUPPORT_CAPTURE_SUBPROCESS_H */
@@ -20,6 +20,7 @@
#include <stdio.h>
#include <support/capture_subprocess.h>
#include <support/check.h>
+#include <sys/wait.h>
static void
print_context (const char *context, bool *failed)
@@ -31,9 +32,22 @@ print_context (const char *context, bool *failed)
printf ("error: subprocess failed: %s\n", context);
}
+static void
+print_actual_status (struct support_capture_subprocess *proc)
+{
+ if (WIFEXITED (proc->status))
+ printf ("error: actual exit status: %d [0x%x]\n",
+ WEXITSTATUS (proc->status), proc->status);
+ else if (WIFSIGNALED (proc->status))
+ printf ("error: actual termination signal: %d [0x%x]\n",
+ WTERMSIG (proc->status), proc->status);
+ else
+ printf ("error: actual undecoded exit status: [0x%x]\n", proc->status);
+}
+
void
support_capture_subprocess_check (struct support_capture_subprocess *proc,
- const char *context, int status,
+ const char *context, int status_or_signal,
int allowed)
{
TEST_VERIFY ((allowed & sc_allow_none)
@@ -44,11 +58,28 @@ support_capture_subprocess_check (struct support_capture_subprocess *proc,
|| (allowed & sc_allow_stderr))));
bool failed = false;
- if (proc->status != status)
+ if (status_or_signal >= 0)
{
- print_context (context, &failed);
- printf ("error: expected exit status: %d\n", status);
- printf ("error: actual exit status: %d\n", proc->status);
+ /* Expect regular termination. */
+ if (!(WIFEXITED (proc->status)
+ && WEXITSTATUS (proc->status) == status_or_signal))
+ {
+ print_context (context, &failed);
+ printf ("error: expected exit status: %d\n", status_or_signal);
+ print_actual_status (proc);
+ }
+ }
+ else
+ {
+ /* status_or_signal < 0. Expect termination by signal. */
+ if (!(WIFSIGNALED (proc->status)
+ && WTERMSIG (proc->status) == -status_or_signal))
+ {
+ print_context (context, &failed);
+ printf ("error: expected termination signal: %d\n",
+ -status_or_signal);
+ print_actual_status (proc);
+ }
}
if (!(allowed & sc_allow_stdout) && proc->out.length != 0)
{
@@ -168,15 +168,29 @@ do_test (void)
= support_capture_subprocess (callback, &test);
check_stream ("stdout", &result.out, test.out);
check_stream ("stderr", &result.err, test.err);
+
+ /* Allowed output for support_capture_subprocess_check. */
+ int check_allow = 0;
+ if (lengths[length_idx_stdout] > 0)
+ check_allow |= sc_allow_stdout;
+ if (lengths[length_idx_stderr] > 0)
+ check_allow |= sc_allow_stderr;
+ if (check_allow == 0)
+ check_allow = sc_allow_none;
+
if (test.signal != 0)
{
TEST_VERIFY (WIFSIGNALED (result.status));
TEST_VERIFY (WTERMSIG (result.status) == test.signal);
+ support_capture_subprocess_check (&result, "signal",
+ -SIGTERM, check_allow);
}
else
{
TEST_VERIFY (WIFEXITED (result.status));
TEST_VERIFY (WEXITSTATUS (result.status) == test.status);
+ support_capture_subprocess_check (&result, "exit",
+ test.status, check_allow);
}
support_capture_subprocess_free (&result);
free (test.out);