[v1,1/1] assert: ensure posix compliance, add tests for such
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Build passed
|
redhat-pt-bot/TryBot-32bit |
fail
|
Patch caused testsuite regressions
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
fail
|
Test failed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
fail
|
Test failed
|
Commit Message
Fix assert.c so that even the fallback
case conforms to POSIX, although not exactly the same as
the default case so a test can tell the difference.
Add a test that verifies that abort is called, and that the
message printed to stderr has all the info that POSIX requires.
Verify this even when malloc isn't usable.
Comments
* DJ Delorie:
> diff --git a/assert/assert.c b/assert/assert.c
> index c29629f5f6..f1711f6995 100644
> --- a/assert/assert.c
> +++ b/assert/assert.c
> @@ -87,8 +87,28 @@ __assert_fail_base (const char *fmt, const char *assertion, const char *file,
> else
> {
> /* At least print a minimal message. */
> - static const char errstr[] = "Unexpected error.\n";
> - __libc_write (STDERR_FILENO, errstr, sizeof (errstr) - 1);
> + char linebuf[100];
> + sprintf(linebuf, "%d", line);
> +#define W(s) __libc_write (STDERR_FILENO, s, sizeof (s) - 1);
> +#define WS(s) __libc_write (STDERR_FILENO, s, strlen (s));
> + if (__progname)
> + {
> + WS(__progname);
> + W(": ");
> + }
> + WS(file);
> + W(":");
> + WS(linebuf);
> + W(": ")
> + if (function)
> + {
> + WS(function);
> + W(": ");
> + }
> + W("Assertion `");
> + WS(assertion);
> + /* Intentionally different for testing purposes. */
> + W("' failed\n");
> }
I think we should do a single write system call (or perhaps use writev),
so that the output is less likely to get interleaved.
But we really should avoid calling malloc (or the full vfprintf) in this
code path. We should also avoid gettext, by looking up the translation
upfront. And we definitely should avoid %n in the format strings.
Thanks,
Florian
Florian Weimer <fweimer@redhat.com> writes:
> I think we should do a single write system call
This is the branch where malloc has already failed; we can't reliably
allocate a buffer.
> (or perhaps use writev),
Is that portable to Hurd?
> so that the output is less likely to get interleaved.
I can't see multiple write() calls being interleaved any more than just
the one. Plus, this is the rarely-used "malloc has failed" case so I
worry less about such things. Previously, we just wrote out "Unexpected
error" here, after all.
> And we definitely should avoid %n in the format strings.
If you mean %d, I thought of that, but historically sprintf doesn't call
malloc for such things as a buffer has been provided. The alternative
is to hack in a small itoa function, but again, this code is called too
rarely to bother.
@@ -38,6 +38,7 @@ tests := \
test-assert-perr \
tst-assert-c++ \
tst-assert-g++ \
+ test-assert-2 \
# tests
ifeq ($(have-cxx-thread_local),yes)
@@ -87,8 +87,28 @@ __assert_fail_base (const char *fmt, const char *assertion, const char *file,
else
{
/* At least print a minimal message. */
- static const char errstr[] = "Unexpected error.\n";
- __libc_write (STDERR_FILENO, errstr, sizeof (errstr) - 1);
+ char linebuf[100];
+ sprintf(linebuf, "%d", line);
+#define W(s) __libc_write (STDERR_FILENO, s, sizeof (s) - 1);
+#define WS(s) __libc_write (STDERR_FILENO, s, strlen (s));
+ if (__progname)
+ {
+ WS(__progname);
+ W(": ");
+ }
+ WS(file);
+ W(":");
+ WS(linebuf);
+ W(": ")
+ if (function)
+ {
+ WS(function);
+ W(": ");
+ }
+ W("Assertion `");
+ WS(assertion);
+ /* Intentionally different for testing purposes. */
+ W("' failed\n");
}
abort ();
new file mode 100644
@@ -0,0 +1,166 @@
+/* Test assert's compliance with POSIX requirements.
+ Copyright (C) 2024 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+/* This test verifies that a failed assertion acts in accordance with
+ the POSIX requirements, despite any internal failures. We do so by
+ calling test routines via fork() and monitoring their exit codes
+ and stderr, while possibly forcing internal functions (malloc) to
+ fail. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <dlfcn.h>
+#include <string.h>
+#include <signal.h>
+
+#undef NDEBUG
+#include <assert.h>
+
+#include <support/check.h>
+#include <support/support.h>
+#include <support/capture_subprocess.h>
+#include <support/xstdio.h>
+
+/* We need to be able to make malloc() "fail" so that __asprintf
+ fails. */
+
+void * (*next_malloc)(size_t sz) = 0;
+static volatile bool fail_malloc = 0;
+
+void *
+malloc (size_t sz)
+{
+ if (fail_malloc)
+ return NULL;
+ if (next_malloc == 0)
+ next_malloc = dlsym (RTLD_NEXT, "malloc");
+ if (next_malloc == 0)
+ return NULL;
+ return next_malloc (sz);
+}
+
+/* We can tell if abort() is called by looking for the custom return
+ value. */
+
+void
+abort_handler(int s)
+{
+ _exit(5);
+}
+
+static int do_lineno;
+static const char *do_funcname;
+
+/* Hack to get the line of the assert. */
+#define GET_INFO_1(l) \
+ if (closure != NULL) \
+ { \
+ do_lineno = l; \
+ do_funcname = __func__; \
+ return; \
+ }
+#define GET_INFO() GET_INFO_1(__LINE__+1)
+
+/* These are the two test cases. */
+
+static void
+test_assert_normal (void *closure)
+{
+ if (closure == NULL)
+ signal (SIGABRT, abort_handler);
+
+ GET_INFO ();
+ assert (1 == 2);
+}
+
+
+static void
+test_assert_nomalloc (void *closure)
+{
+ if (closure == NULL)
+ {
+ signal (SIGABRT, abort_handler);
+ fail_malloc = 1;
+ }
+
+ GET_INFO ();
+ assert (1 == 2);
+}
+
+static void
+check_posix (const char *buf, int rv, int line,
+ const char *funcname, const char *testarg)
+{
+ /* POSIX requires that a failed assertion write a diagnostic to
+ stderr and call abort. The diagnostic must include the filename,
+ line number, and function where the assertion failed, along with
+ the text of the assert() argument.
+ */
+ char linestr[100];
+
+ sprintf (linestr, "%d", line);
+
+ /* If abort is called, our handler will return 5. */
+ TEST_VERIFY (rv == 5);
+
+ TEST_VERIFY (strstr (buf, __FILE__) != NULL);
+ TEST_VERIFY (strstr (buf, linestr) != NULL);
+ TEST_VERIFY (strstr (buf, funcname) != NULL);
+ TEST_VERIFY (strstr (buf, testarg) != NULL);
+
+}
+
+static void
+one_test (void (*func)(void *), int which_path)
+{
+ struct support_capture_subprocess sp;
+ int rv;
+
+ func (&do_lineno);
+ printf("running test for %s, line %d\n", do_funcname, do_lineno);
+
+ sp = support_capture_subprocess (func, NULL);
+ rv = WEXITSTATUS (sp.status);
+
+ check_posix (sp.err.buffer, rv, do_lineno, do_funcname, "1 == 2");
+
+ /* Look for intentional subtle differences in messages to verify
+ that the intended code path was taken. */
+ switch (which_path)
+ {
+ case 0:
+ TEST_VERIFY (strstr (sp.err.buffer, "failed.\n") != NULL);
+ break;
+ case 1:
+ TEST_VERIFY (strstr (sp.err.buffer, "failed\n") != NULL);
+ break;
+ }
+
+ support_capture_subprocess_free (&sp);
+}
+
+static int
+do_test(void)
+{
+ one_test (test_assert_normal, 0);
+ one_test (test_assert_nomalloc, 1);
+
+ return 0;
+}
+
+#include <support/test-driver.c>