Skip to content

Conversation

@alejandro-colomar
Copy link
Collaborator

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

This is a subset of #1383.

@ikerexxe This is the minimum fix.


tests/unit/test_exit_if_null.c: Test through XMALLOC() instead of xaprintf()

Both are indirect tests for exit_if_null(), but through XMALLOC() we
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: #1382
Reported-by: @infinisil
Tested-by: @infinisil


Revisions:

v2
  • Test that pointer is not null.
$ git rd 
1:  b0291e99e = 1:  b0291e99e tests/unit/: Use more generic strings and names for testing exit_if_null()
2:  04617ca48 ! 2:  aed741a4c tests/unit/test_exit_if_null.c: Test through XMALLOC() instead of xaprintf()
    @@ tests/unit/test_exit_if_null.c: test_exit_if_null_ok(void **state)
        free(p);
     +
     +  p = XMALLOC(0, char);
    ++  assert_true(p != NULL);
     +  free(p);
      }
v2b
$ git rd 
1:  b0291e99e ! 1:  569d9787c tests/unit/: Use more generic strings and names for testing exit_if_null()
    @@ Commit message
         test file and functions, and make strings more generic.
     
         Tested-by: Silvan Mosberger <github@infinisil.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##
2:  aed741a4c ! 2:  517e01961 tests/unit/test_exit_if_null.c: Test through XMALLOC() instead of xaprintf()
    @@ Commit message
         Closes: <https://github.com/shadow-maint/shadow/issues/1382>
         Reported-by: Silvan Mosberger <github@infinisil.com>
         Tested-by: Silvan Mosberger <github@infinisil.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##

@alejandro-colomar alejandro-colomar linked an issue Nov 5, 2025 that may be closed by this pull request
@alejandro-colomar alejandro-colomar marked this pull request as ready for review November 5, 2025 12:24
@ikerexxe
Copy link
Collaborator

ikerexxe commented Nov 5, 2025

The code changes look good, but the PR lacks sufficient context and explanation. Do you mind adding a proper title and description to the PR? The information that is in the commits themselves should be sufficient. I don't want to come back in the future and ask myself why I approved this PR and try to understand what was happening.

@alejandro-colomar alejandro-colomar changed the title X2 tests/unit/test_exit_if_null.c: Test through XMALLOC() instead of xaprintf() Nov 5, 2025
@alejandro-colomar
Copy link
Collaborator Author

The code changes look good, but the PR lacks sufficient context and explanation. Do you mind adding a proper title and description to the PR? The information that is in the commits themselves should be sufficient. I don't want to come back in the future and ask myself why I approved this PR and try to understand what was happening.

Done. Thanks!

Copy link
Collaborator

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

I feel more comfortable with merging the bare minimum. Thank you!

…ull()

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>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
…rintf()

Both are indirect tests for exit_if_null(), but through XMALLOC() we
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: <shadow-maint#1382>
Reported-by: Silvan Mosberger <github@infinisil.com>
Tested-by: Silvan Mosberger <github@infinisil.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
Copy link
Collaborator Author

Thanks! I'll merge.

@alejandro-colomar alejandro-colomar merged commit 7d4362f into shadow-maint:master Nov 6, 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

2 participants