[v1,1/1] assert: ensure posix compliance, add tests for such

Message ID xno72h8t4i.fsf@greed.delorie.com
State New
Headers
Series [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

DJ Delorie Nov. 14, 2024, 8:20 p.m. UTC
  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

Florian Weimer Nov. 15, 2024, 12:57 p.m. UTC | #1
* 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
  
DJ Delorie Nov. 15, 2024, 7:08 p.m. UTC | #2
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.
  

Patch

diff --git a/assert/Makefile b/assert/Makefile
index 35dc908ddb..6f2717c80a 100644
--- a/assert/Makefile
+++ b/assert/Makefile
@@ -38,6 +38,7 @@  tests := \
   test-assert-perr \
   tst-assert-c++ \
   tst-assert-g++ \
+  test-assert-2 \
   # tests
 
 ifeq ($(have-cxx-thread_local),yes)
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");
     }
 
   abort ();
diff --git a/assert/test-assert-2.c b/assert/test-assert-2.c
new file mode 100644
index 0000000000..99f8f683e8
--- /dev/null
+++ b/assert/test-assert-2.c
@@ -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>