Skip to content

Conversation

@alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Nov 4, 2025

See also: #1382

Cc: @infinisil, @ikerexxe


@infinisil, would you mind testing if after this patch you still see an error in Nix?


Revisions:

v1b
$ git rd 
1:  d04f146b4 ! 1:  c35e83497 tests/unit/test_xaprintf.c: Use assert_string_equal()
    @@ Commit message
     
         This produces more useful test results.
     
    +    Tested-by: Silvan Mosberger <github@infinisil.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##
2:  d0765022a ! 2:  d0ff90814 tests/unit/: Use more generic strings and names for testing exit_if_null()
    @@ Commit message
         This test is actually for exit_if_null(), not xaprintf().  Rename the
         test file and functions, and make strings more generic.
     
    +    Tested-by: Silvan Mosberger <github@infinisil.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##
3:  e6a2a161b ! 3:  ac6c58511 tests/unit/test_exit_if_null.c: Test through XMALLOC() instead of xaprintf()
    @@ Commit message
         can test it more robustly, as we don't need to wrap vasprintf(3) to
         make it fail.  It's trivial to make MALLOC(3) fail: pass a huge size.
     
    +    The tests with xaprintf() were failing on Nix.  I suspect the compiler
    +    was inlining aggressively, and as a result, the interposition of
    +    vasprintf(3) in cmocka wasn't actually working.  The approach with
    +    XMALLOC() seems to work on Nix, as we don't need to interpose malloc(3).
    +    We still need to interpose exit(3), but for some reason that works fine.
    +
    +    Closes: <https://github.com/shadow-maint/shadow/issues/1382>
    +    Reported-by: Silvan Mosberger <github@infinisil.com>
    +    Tested-by: Silvan Mosberger <github@infinisil.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##
4:  4d9f17a6d ! 4:  6f516077a tests/unit/: Unname unused parameters in callbacks
    @@ Commit message
     
         This silences diagnostics about unused parameters.
     
    +    Tested-by: Silvan Mosberger <github@infinisil.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/test_adds.c ##
v1c
  • Reorder commits.
$ git rd --creation-factor=99
2:  d0ff90814 ! 1:  b0291e99e tests/unit/: Use more generic strings and names for testing exit_if_null()
    @@ tests/unit/Makefile.am: test_typetraits_LDADD = \
          ../../lib/exit_if_null.c \
          ../../lib/shadowlog.c \
          ../../lib/string/sprintf/aprintf.c \
    +     ../../lib/string/strcmp/streq.c \
     -    test_xaprintf.c \
     +    test_exit_if_null.c \
          $(NULL)
    @@ tests/unit/test_exit_if_null.c: test_xaprintf_exit(void **state)
                assert_unreachable();
                break;
        case EXIT_CALLED:
    --          assert_string_equal(p, "xaprintf_called");
    -+          assert_string_equal(p, "called");
    +-          assert_true(streq(p, "xaprintf_called"));
    ++          assert_true(streq(p, "called"));
                p = "test_ok";
                break;
        default:
3:  ac6c58511 ! 2:  04617ca48 tests/unit/test_exit_if_null.c: Test through XMALLOC() instead of xaprintf()
    @@ tests/unit/Makefile.am: test_typetraits_LDADD = \
          ../../lib/shadowlog.c \
     -    ../../lib/string/sprintf/aprintf.c \
     +    ../../lib/alloc/malloc.c \
    +     ../../lib/string/strcmp/streq.c \
          test_exit_if_null.c \
          $(NULL)
    - test_exit_if_null_CFLAGS = \
    +@@ tests/unit/Makefile.am: test_exit_if_null_CFLAGS = \
          $(AM_CFLAGS) \
          $(NULL)
      test_exit_if_null_LDFLAGS = \
    @@ tests/unit/test_exit_if_null.c
      #include <cmocka.h>
      
     +#include "sizeof.h"
    -+
    + #include "string/strcmp/streq.h"
    + 
      
     -#define smock()               _Generic(mock(), uintmax_t: (intmax_t) mock())
      #define assert_unreachable()  assert_true(0)
1:  c35e83497 ! 3:  35bbe9016 tests/unit/test_xaprintf.c: Use assert_string_equal()
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##
    -@@ tests/unit/Makefile.am: test_xaprintf_SOURCES = \
    +@@ tests/unit/Makefile.am: test_exit_if_null_SOURCES = \
          ../../lib/exit_if_null.c \
          ../../lib/shadowlog.c \
    -     ../../lib/string/sprintf/aprintf.c \
    +     ../../lib/alloc/malloc.c \
     -    ../../lib/string/strcmp/streq.c \
    -     test_xaprintf.c \
    +     test_exit_if_null.c \
          $(NULL)
    - test_xaprintf_CFLAGS = \
    + test_exit_if_null_CFLAGS = \
     
    - ## tests/unit/test_xaprintf.c ##
    + ## tests/unit/test_exit_if_null.c ##
     @@
    - #include <stdint.h>  // Required by <cmocka.h>
      #include <cmocka.h>
      
    + #include "sizeof.h"
     -#include "string/strcmp/streq.h"
    --
      
    - #define smock()               _Generic(mock(), uintmax_t: (intmax_t) mock())
    + 
      #define assert_unreachable()  assert_true(0)
    -@@ tests/unit/test_xaprintf.c: test_xaprintf_exit(void **state)
    +@@ tests/unit/test_exit_if_null.c: test_exit_if_null_exit(void **state)
                assert_unreachable();
                break;
        case EXIT_CALLED:
    --          assert_true(streq(p, "xaprintf_called"));
    -+          assert_string_equal(p, "xaprintf_called");
    +-          assert_true(streq(p, "called"));
    ++          assert_string_equal(p, "called");
                p = "test_ok";
                break;
        default:
    -@@ tests/unit/test_xaprintf.c: test_xaprintf_exit(void **state)
    +@@ tests/unit/test_exit_if_null.c: test_exit_if_null_exit(void **state)
                break;
        }
      
4:  6f516077a = 4:  21a7b4014 tests/unit/: Unname unused parameters in callbacks
v2
$ git rd
1:  b0291e99e < -:  --------- tests/unit/: Use more generic strings and names for testing exit_if_null()
2:  04617ca48 < -:  --------- tests/unit/test_exit_if_null.c: Test through XMALLOC() instead of xaprintf()
3:  35bbe9016 = 1:  4f8b463f3 tests/unit/test_xaprintf.c: Use assert_string_equal()
4:  21a7b4014 = 2:  0ebdbeea3 tests/unit/: Unname unused parameters in callbacks
v2b
  • Rebase
$ git rd 
1:  4f8b463f3 = 1:  b481b121d tests/unit/test_xaprintf.c: Use assert_string_equal()
2:  0ebdbeea3 ! 2:  bc367c8ec tests/unit/: Unname unused parameters in callbacks
    @@ tests/unit/test_snprintf.c
      #include "string/sprintf/snprintf.h"
      
      
    --static void test_SNPRINTF_trunc(void **state);
    --static void test_SNPRINTF_ok(void **state);
    -+static void test_SNPRINTF_trunc(void **);
    -+static void test_SNPRINTF_ok(void **);
    +-static void test_stprintf_a_trunc(void **state);
    +-static void test_stprintf_a_ok(void **state);
    ++static void test_stprintf_a_trunc(void **);
    ++static void test_stprintf_a_ok(void **);
      
      
      int
    @@ tests/unit/test_snprintf.c: main(void)
      
      
      static void
    --test_SNPRINTF_trunc(void **state)
    -+test_SNPRINTF_trunc(void **)
    +-test_stprintf_a_trunc(void **state)
    ++test_stprintf_a_trunc(void **)
      {
        char  buf[countof("foo")];
      
    -@@ tests/unit/test_snprintf.c: test_SNPRINTF_trunc(void **state)
    +@@ tests/unit/test_snprintf.c: test_stprintf_a_trunc(void **state)
      
      
      static void
    --test_SNPRINTF_ok(void **state)
    -+test_SNPRINTF_ok(void **)
    +-test_stprintf_a_ok(void **state)
    ++test_stprintf_a_ok(void **)
      {
        char  buf[countof("foo")];
      
    @@ tests/unit/test_strncpy.c
      #include "string/strcpy/strncpy.h"
      
      
    --static void test_STRNCPY_trunc(void **state);
    --static void test_STRNCPY_fit(void **state);
    --static void test_STRNCPY_pad(void **state);
    -+static void test_STRNCPY_trunc(void **);
    -+static void test_STRNCPY_fit(void **);
    -+static void test_STRNCPY_pad(void **);
    +-static void test_strncpy_a_trunc(void **state);
    +-static void test_strncpy_a_fit(void **state);
    +-static void test_strncpy_a_pad(void **state);
    ++static void test_strncpy_a_trunc(void **);
    ++static void test_strncpy_a_fit(void **);
    ++static void test_strncpy_a_pad(void **);
      
      
      int
    @@ tests/unit/test_strncpy.c: main(void)
      
      
      static void
    --test_STRNCPY_trunc(void **state)
    -+test_STRNCPY_trunc(void **)
    +-test_strncpy_a_trunc(void **state)
    ++test_strncpy_a_trunc(void **)
      {
        char  buf[3];
      
    -@@ tests/unit/test_strncpy.c: test_STRNCPY_trunc(void **state)
    +@@ tests/unit/test_strncpy.c: test_strncpy_a_trunc(void **state)
      
      
      static void
    --test_STRNCPY_fit(void **state)
    -+test_STRNCPY_fit(void **)
    +-test_strncpy_a_fit(void **state)
    ++test_strncpy_a_fit(void **)
      {
        char  buf[3];
      
    -@@ tests/unit/test_strncpy.c: test_STRNCPY_fit(void **state)
    +@@ tests/unit/test_strncpy.c: test_strncpy_a_fit(void **state)
      
      
      static void
    --test_STRNCPY_pad(void **state)
    -+test_STRNCPY_pad(void **)
    +-test_strncpy_a_pad(void **state)
    ++test_strncpy_a_pad(void **)
      {
        char  buf[3];
      
    @@ tests/unit/test_strtcpy.c
      #include "string/strcpy/strtcpy.h"
      
      
    --static void test_STRTCPY_trunc(void **state);
    --static void test_STRTCPY_ok(void **state);
    -+static void test_STRTCPY_trunc(void **);
    -+static void test_STRTCPY_ok(void **);
    +-static void test_strtcpy_a_trunc(void **state);
    +-static void test_strtcpy_a_ok(void **state);
    ++static void test_strtcpy_a_trunc(void **);
    ++static void test_strtcpy_a_ok(void **);
      
      
      int
    @@ tests/unit/test_strtcpy.c: main(void)
      
      
      static void
    --test_STRTCPY_trunc(void **state)
    -+test_STRTCPY_trunc(void **)
    +-test_strtcpy_a_trunc(void **state)
    ++test_strtcpy_a_trunc(void **)
      {
        char  buf[countof("foo")];
      
    -@@ tests/unit/test_strtcpy.c: test_STRTCPY_trunc(void **state)
    +@@ tests/unit/test_strtcpy.c: test_strtcpy_a_trunc(void **state)
      
      
      static void
    --test_STRTCPY_ok(void **state)
    -+test_STRTCPY_ok(void **)
    +-test_strtcpy_a_ok(void **state)
    ++test_strtcpy_a_ok(void **)
      {
        char  buf[countof("foo")];
      
v2c
  • Document in the commit message how assert_string_equal() is more informative. [@hallyn ]
$ git rd 
1:  b481b121d ! 1:  50c37e833 tests/unit/test_xaprintf.c: Use assert_string_equal()
    @@ Commit message
     
         This produces more useful test results.
     
    +    With assert_true(streq(...)), we only see the line of code that
    +    triggered the failure, while assert_string_equal() shows the contents
    +    of the strings.  See the following example:
    +
    +            alx@devuan:~/tmp$ cat cmocka.c
    +            #include <string.h>
    +
    +            #include <stdarg.h>
    +            #include <stddef.h>
    +            #include <setjmp.h>
    +            #include <stdint.h>
    +            #include <cmocka.h>
    +
    +            #define streq(a,b)  (!strcmp(a,b))
    +
    +            static void a(void **)
    +            {
    +                    const char *s = "foo";
    +
    +                    assert_true(streq(s, "bar"));
    +            }
    +
    +            static void b(void **)
    +            {
    +                    const char *s = "foo";
    +
    +                    assert_string_equal(s, "bar");
    +            }
    +
    +            int
    +            main(void)
    +            {
    +
    +                    const struct CMUnitTest tests[] = {
    +                            cmocka_unit_test(a),
    +                            cmocka_unit_test(b),
    +                    };
    +
    +                    return cmocka_run_group_tests(tests, NULL, NULL);
    +            }
    +            alx@devuan:~/tmp$ gcc cmocka.c -lcmocka
    +            alx@devuan:~/tmp$ ./a.out
    +            [==========] tests: Running 2 test(s).
    +            [ RUN      ] a
    +            [  ERROR   ] --- streq(s, "bar")
    +            [   LINE   ] --- cmocka.c:15: error: Failure!
    +            [  FAILED  ] a
    +            [ RUN      ] b
    +            [  ERROR   ] --- "foo" != "bar"
    +            [   LINE   ] --- cmocka.c:22: error: Failure!
    +            [  FAILED  ] b
    +            [==========] tests: 2 test(s) run.
    +            [  PASSED  ] 0 test(s).
    +            [  FAILED  ] tests: 2 test(s), listed below:
    +            [  FAILED  ] a
    +            [  FAILED  ] b
    +
    +             2 FAILED TEST(S)
    +
         Tested-by: Silvan Mosberger <github@infinisil.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
2:  bc367c8ec = 2:  3f93f2f70 tests/unit/: Unname unused parameters in callbacks

Copy link

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that this works under Nix, thank you!

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Nov 4, 2025

I can confirm that this works under Nix, thank you!

Hmmmmmm, then I suspect it was due to inlining vasprintf(3). It was probably being inlined, and thus our interposition didn't work, so we couldn't make it fail in the test. Thanks!

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Nov 4, 2025

I've added Reported-by and Tested-by tags for you, @infinisil .

@ikerexxe
Copy link
Collaborator

ikerexxe commented Nov 5, 2025

I see many changes that are unrelated to the fix itself. Since you already know what's the exact fix, can you provide just that thing in a PR?

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Nov 5, 2025

I see many changes that are unrelated to the fix itself. Since you already know what's the exact fix, can you provide just that thing in a PR?

The fix is the third commit (the commit that has the Closes: tag). But the second commit is necessary to make the third commit.

The fourth commit was unnecessary. I only added it because debugging this PR was painful, as there are many warnings in the tests, which made it difficult to find the actual errors. I could remove the 1st and 4th commits, if you want.

@hallyn
Copy link
Member

hallyn commented Nov 28, 2025

Can you update the first commit message with an example of a more useful test result?

@alejandro-colomar
Copy link
Collaborator Author

Can you update the first commit message with an example of a more useful test result?

Yup.

I had never run cmocka manually, as it seemed complex, but your message prompted me to do so. Here it is, for posterity:

alx@devuan:~/tmp$ cat cmocka.c 
#include <string.h>

#include <stdarg.h>
#include <stddef.h>
#include <setjmp.h>
#include <stdint.h>
#include <cmocka.h>

#define streq(a,b)  (!strcmp(a,b))

static void a(void **)
{
	const char *s = "foo";

	assert_true(streq(s, "bar"));
}

static void b(void **)
{
	const char *s = "foo";

	assert_string_equal(s, "bar");
}

int
main(void)
{

	const struct CMUnitTest tests[] = {
		cmocka_unit_test(a),
		cmocka_unit_test(b),
	};

	return cmocka_run_group_tests(tests, NULL, NULL);
}
alx@devuan:~/tmp$ gcc cmocka.c -lcmocka
alx@devuan:~/tmp$ ./a.out 
[==========] tests: Running 2 test(s).
[ RUN      ] a
[  ERROR   ] --- streq(s, "bar")
[   LINE   ] --- cmocka.c:15: error: Failure!
[  FAILED  ] a
[ RUN      ] b
[  ERROR   ] --- "foo" != "bar"
[   LINE   ] --- cmocka.c:22: error: Failure!
[  FAILED  ] b
[==========] tests: 2 test(s) run.
[  PASSED  ] 0 test(s).
[  FAILED  ] tests: 2 test(s), listed below:
[  FAILED  ] a
[  FAILED  ] b

 2 FAILED TEST(S)

This will allow us to debug issues with cmocka without having to tun the test suite.

For your question, see that in test a, the error report only shows the line of code, which doesn't show the contents of the string s. In test b, it shows the contents of both strings.

I'll update the commit message with that.

This produces more useful test results.

With assert_true(streq(...)), we only see the line of code that
triggered the failure, while assert_string_equal() shows the contents
of the strings.  See the following example:

	alx@devuan:~/tmp$ cat cmocka.c
	#include <string.h>

	#include <stdarg.h>
	#include <stddef.h>
	#include <setjmp.h>
	#include <stdint.h>
	#include <cmocka.h>

	#define streq(a,b)  (!strcmp(a,b))

	static void a(void **)
	{
		const char *s = "foo";

		assert_true(streq(s, "bar"));
	}

	static void b(void **)
	{
		const char *s = "foo";

		assert_string_equal(s, "bar");
	}

	int
	main(void)
	{

		const struct CMUnitTest tests[] = {
			cmocka_unit_test(a),
			cmocka_unit_test(b),
		};

		return cmocka_run_group_tests(tests, NULL, NULL);
	}
	alx@devuan:~/tmp$ gcc cmocka.c -lcmocka
	alx@devuan:~/tmp$ ./a.out
	[==========] tests: Running 2 test(s).
	[ RUN      ] a
	[  ERROR   ] --- streq(s, "bar")
	[   LINE   ] --- cmocka.c:15: error: Failure!
	[  FAILED  ] a
	[ RUN      ] b
	[  ERROR   ] --- "foo" != "bar"
	[   LINE   ] --- cmocka.c:22: error: Failure!
	[  FAILED  ] b
	[==========] tests: 2 test(s) run.
	[  PASSED  ] 0 test(s).
	[  FAILED  ] tests: 2 test(s), listed below:
	[  FAILED  ] a
	[  FAILED  ] b

	 2 FAILED TEST(S)

Tested-by: Silvan Mosberger <github@infinisil.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This silences diagnostics about unused parameters.

Tested-by: Silvan Mosberger <github@infinisil.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
Copy link
Collaborator Author

Can you update the first commit message with an example of a more useful test result?

Done.

@hallyn hallyn merged commit aee5c0f into shadow-maint:master Nov 28, 2025
11 checks passed
@alejandro-colomar alejandro-colomar self-assigned this Dec 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_xaprintf.c failure

4 participants