Add unit tests for xasprintf() by alejandro-colomar · Pull Request #816 · shadow-maint/shadow

alejandro-colomar · 2023-10-06T15:51:14Z

@ikerexxe I'll add the tests in this separate PR, to avoid cluttering the discussion for the other changes.

Signed-off-by: Alejandro Colomar <alx@kernel.org>

As other x...() wrappers around functions that allocate, these wrappers are like [v]asprintf(3), but exit on failure. Signed-off-by: Alejandro Colomar <alx@kernel.org>

Signed-off-by: Alejandro Colomar <alx@kernel.org>

asprintf(3) is non-standard, but is provided by GNU, the BSDs, and musl. That makes it portable enough for us to use. This function is much simpler than the burdensome code for allocating the right size. Being simpler, it's thus safer. I took the opportunity to fix the style to my preferred one in the definitions of variables used in these calls, and also in the calls to free(3) with these pointers. That isn't gratuituous, but has a reason: it makes those appear in the diff for this patch, which helps review it. Oh, well, I had an excuse :) Signed-off-by: Alejandro Colomar <alx@kernel.org>

Signed-off-by: Alejandro Colomar <alx@kernel.org>

This is not just a style issue. This should be a hard error, and never compile. ISO C89 already had this feature as deprecated. ISO C99 removed this deprecated feature, for good reasons. If we compile ignoring this warning, shadow is not going to behave well. Cc: Sam James <sam@gentoo.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar · 2023-10-06T15:52:32Z

@ikerexxe Do you know why I get linker errors about main()? I really don't understand what I'm doing in the autotools code for it.

Edit: ahh, it was just a typo!

alejandro-colomar · 2023-10-18T12:01:33Z

v2 changes:

Add missing brace. [@ikerexxe ]
Move out of if ENABLE_LOGIND. [@ikerexxe ]

$ git range-diff 4b4c7e9f gh/xasprintf_test xasprintf_test 
1:  b82f3e93 ! 1:  664f597c tests/unit/test_xasprintf_x.c: Test error path of xasprintf()
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##
    -@@ tests/unit/Makefile.am: check_PROGRAMS =
    +@@ tests/unit/Makefile.am: AM_CPPFLAGS = -I$(top_srcdir)/lib -I$(top_srcdir)
    + if HAVE_CMOCKA
    + TESTS = $(check_PROGRAMS)
    + 
    +-check_PROGRAMS =
    ++check_PROGRAMS = \
    ++    test_xasprintf_x
      
      if ENABLE_LOGIND
    - check_PROGRAMS += \
    --    test_logind
    -+    test_logind \
    -+    test_xasprintf_x
    - endif # ENABLE_LOGIND
    - 
      check_PROGRAMS += \
     @@ tests/unit/Makefile.am: test_logind_LDADD = \
          $(CMOCKA_LIBS) \
    @@ tests/unit/test_xasprintf_x.c (new)
     +
     +  len = 0;
     +
    -+  switch (setjmp(jmpb))
    ++  switch (setjmp(jmpb)) {
     +  case 0:
     +          len = -36;
     +          len = xasprintf(&p, "foo%s", "bar");
2:  7f98c6b8 ! 2:  0cef7086 tests/unit/test_xasprintf.c: Test non-error path of xasprintf(3)
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##
    -@@ tests/unit/Makefile.am: check_PROGRAMS =
    - if ENABLE_LOGIND
    - check_PROGRAMS += \
    -     test_logind \
    +@@ tests/unit/Makefile.am: if HAVE_CMOCKA
    + TESTS = $(check_PROGRAMS)
    + 
    + check_PROGRAMS = \
     +    test_xasprintf \
          test_xasprintf_x
    - endif # ENABLE_LOGIND
      
    + if ENABLE_LOGIND
     @@ tests/unit/Makefile.am: test_logind_LDADD = \
          $(LIBSYSTEMD) \
          $(NULL)

alejandro-colomar · 2023-10-18T12:07:04Z

v3 changes:

Add missing include

$ git range-diff asprintf gh/xasprintf_test xasprintf_test 
1:  664f597c ! 1:  d65036f8 tests/unit/test_xasprintf_x.c: Test error path of xasprintf()
    @@ tests/unit/test_xasprintf_x.c (new)
     + */
     +
     +
    ++#include <setjmp.h>
     +#include <stddef.h>
     +#include <stdlib.h>
     +
2:  0cef7086 = 2:  dfaa64f4 tests/unit/test_xasprintf.c: Test non-error path of xasprintf(3)

alejandro-colomar · 2023-10-18T12:16:58Z

v4 changes:

Add missing includes required by <cmocka.h>

$ git range-diff asprintf gh/xasprintf_test xasprintf_test 
1:  d65036f8 ! 1:  5266d63c tests/unit/test_xasprintf_x.c: Test error path of xasprintf()
    @@ tests/unit/test_xasprintf_x.c (new)
     +#include <stddef.h>
     +#include <stdlib.h>
     +
    ++#include <stdarg.h>  // Required by <cmocka.h>
    ++#include <stddef.h>  // Required by <cmocka.h>
    ++#include <setjmp.h>  // Required by <cmocka.h>
    ++#include <stdint.h>  // Required by <cmocka.h>
     +#include <cmocka.h>
     +
     +#include "sprintf.h"
2:  dfaa64f4 ! 2:  c23eb2b2 tests/unit/test_xasprintf.c: Test non-error path of xasprintf(3)
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>
     
      ## Commit message ##
    -    tests/unit/test_xasprintf.c: Test non-error path of xasprintf(3)
    +    tests/unit/test_xasprintf.c: Test non-error path of xasprintf()
     
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
    @@ tests/unit/test_xasprintf.c (new)
     +#include <stdlib.h>
     +#include <string.h>
     +
    ++#include <stdarg.h>  // Required by <cmocka.h>
    ++#include <stddef.h>  // Required by <cmocka.h>
    ++#include <setjmp.h>  // Required by <cmocka.h>
    ++#include <stdint.h>  // Required by <cmocka.h>
     +#include <cmocka.h>
     +
     +#include "sprintf.h"

alejandro-colomar · 2023-10-18T12:20:12Z

v5 changes:

Fix syntax error.

$ git range-diff asprintf gh/xasprintf_test xasprintf_test 
1:  5266d63c = 1:  5266d63c tests/unit/test_xasprintf_x.c: Test error path of xasprintf()
2:  c23eb2b2 ! 2:  52116fef tests/unit/test_xasprintf.c: Test non-error path of xasprintf()
    @@ tests/unit/test_xasprintf.c (new)
     +  char  *p;
     +
     +  len = xasprintf(&p, "foo%d%s", 1, "bar");
    -+  assert_int_equal(len, strlen("foo1bar");
    ++  assert_int_equal(len, strlen("foo1bar"));
     +  assert_string_equal(p, "foo1bar");
     +  free(p);
     +}

alejandro-colomar · 2023-10-18T12:25:12Z

v6 changes:

Remove unused LDFLAGS

$ git range-diff asprintf gh/xasprintf_test xasprintf_test 
1:  5266d63c = 1:  5266d63c tests/unit/test_xasprintf_x.c: Test error path of xasprintf()
2:  52116fef ! 2:  dfbd529c tests/unit/test_xasprintf.c: Test non-error path of xasprintf()
    @@ tests/unit/Makefile.am: test_logind_LDADD = \
     +    $(AM_FLAGS) \
     +    $(NULL)
     +test_xasprintf_LDFLAGS = \
    -+    -Wl,-wrap,malloc \
    -+    -Wl,-wrap,realloc \
    -+    -Wl,-wrap,exit \
     +    $(NULL)
     +test_xasprintf_LDADD = \
     +    $(CMOCKA_LIBS) \

alejandro-colomar · 2023-10-18T12:35:11Z

v7 changes:

Remove unused variable
Fix test (6 => -6)

$ git range-diff asprintf gh/xasprintf_test xasprintf_test 
1:  5266d63c ! 1:  daf0f2ee tests/unit/test_xasprintf_x.c: Test error path of xasprintf()
    @@ tests/unit/test_xasprintf_x.c (new)
     +#define assert_unreachable()  assert_true(0)
     +
     +
    -+static int      magic;
     +static jmp_buf  jmpb;
     +
     +
    @@ tests/unit/test_xasprintf_x.c (new)
     +          break;
     +  }
     +
    -+  assert_int_equal(len, 6);
    ++  assert_int_equal(len, -6);
     +}
     +
     +
2:  dfbd529c = 2:  b03aa7e9 tests/unit/test_xasprintf.c: Test non-error path of xasprintf()

alejandro-colomar · 2023-10-18T12:47:05Z

Here's the log of the tests:

==============================================
   shadow 4.14.0: tests/unit/test-suite.log
==============================================

# TOTAL: 2
# PASS:  1
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: test_xasprintf_x
======================

[==========] tests: Running 1 test(s).
[ RUN      ] test_xasprintf_x
[  ERROR   ] --- 0
[   LINE   ] --- test_xasprintf_x.c:65: error: Failure!
[  FAILED  ] test_xasprintf_x
[==========] tests: 1 test(s) run.
[  PASSED  ] 0 test(s).
[  FAILED  ] tests: 1 test(s), listed below:
[  FAILED  ] test_xasprintf_x

 1 FAILED TEST(S)
FAIL test_xasprintf_x (exit status: 1)

The failure seems to be due to reaching line 65, which is assert_unreachable();. I'm not sure why it arrives there. Maybe interposing exit(3) with longjmp(3) isn't so easy.

Or maybe we can't interpose what glibc calls internally within asprintf(3), so maybe the malloc(3) wrapper is not working as intended.

ikerexxe · 2023-10-18T12:55:49Z

The failure seems to be due to reaching line 65, which is assert_unreachable();. I'm not sure why it arrives there. Maybe interposing exit(3) with longjmp(3) isn't so easy.

Or maybe we can't interpose what glibc calls internally within asprintf(3), so maybe the malloc(3) wrapper is not working as intended.

Let's wait for Andreas to answer in the other PR

alejandro-colomar · 2023-10-18T12:58:26Z

The failure seems to be due to reaching line 65, which is assert_unreachable();. I'm not sure why it arrives there. Maybe interposing exit(3) with longjmp(3) isn't so easy.
Or maybe we can't interpose what glibc calls internally within asprintf(3), so maybe the malloc(3) wrapper is not working as intended.

Let's wait for Andreas to answer in the other PR

Sure; let's add a mention here just in case: @cryptomilk

alejandro-colomar · 2023-10-18T13:04:24Z

v8 changes:

Interpose asprintf(3) directly, instead of malloc(3) and realloc(3).

$ git range-diff asprintf gh/xasprintf_test xasprintf_test 
1:  daf0f2ee ! 1:  dbc4285e tests/unit/test_xasprintf_x.c: Test error path of xasprintf()
    @@ tests/unit/Makefile.am: test_logind_LDADD = \
     +    $(AM_FLAGS) \
     +    $(NULL)
     +test_xasprintf_x_LDFLAGS = \
    -+    -Wl,-wrap,malloc \
    -+    -Wl,-wrap,realloc \
    ++    -Wl,-wrap,asprintf \
     +    -Wl,-wrap,exit \
     +    $(NULL)
     +test_xasprintf_x_LDADD = \
    @@ tests/unit/test_xasprintf_x.c (new)
     +/**********************
     + * WRAPPERS
     + **********************/
    -+void *
    -+__wrap_malloc(size_t size)
    ++int
    ++__wrap_asprintf(char **restrict p, const char *restrict fmt, ...)
     +{
    -+  return NULL;
    -+}
    -+
    -+
    -+void *
    -+__wrap_realloc(void *p, size_t size)
    -+{
    -+  return NULL;
    ++  return -1;
     +}
     +
     +
2:  b03aa7e9 = 2:  d3ad213b tests/unit/test_xasprintf.c: Test non-error path of xasprintf()

alejandro-colomar · 2023-10-18T13:08:42Z

Even interposing asprintf(3) directly, the longjmp(3) isn't working. Here's the log:

==============================================
   shadow 4.14.0: tests/unit/test-suite.log
==============================================

# TOTAL: 2
# PASS:  1
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: test_xasprintf_x
======================

[==========] tests: Running 1 test(s).
[ RUN      ] test_xasprintf_x
[  ERROR   ] --- 0
[   LINE   ] --- test_xasprintf_x.c:58: error: Failure!
[  FAILED  ] test_xasprintf_x
[==========] tests: 1 test(s) run.
[  PASSED  ] 0 test(s).
[  FAILED  ] tests: 1 test(s), listed below:
[  FAILED  ] test_xasprintf_x

 1 FAILED TEST(S)
FAIL test_xasprintf_x (exit status: 1)

$ sed -n 58p tests/unit/test_xasprintf_x.c
		assert_unreachable();

EDIT: My bad, I need to interpose vasprintf(3).

Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar · 2023-10-18T13:12:47Z

v9 changes:

Interpose vasprintf(3), not asprintf(3).

$ git range-diff asprintf gh/xasprintf_test xasprintf_test 
1:  dbc4285e ! 1:  74d68259 tests/unit/test_xasprintf_x.c: Test error path of xasprintf()
    @@ tests/unit/Makefile.am: test_logind_LDADD = \
     +    $(AM_FLAGS) \
     +    $(NULL)
     +test_xasprintf_x_LDFLAGS = \
    -+    -Wl,-wrap,asprintf \
    ++    -Wl,-wrap,vasprintf \
     +    -Wl,-wrap,exit \
     +    $(NULL)
     +test_xasprintf_x_LDADD = \
    @@ tests/unit/test_xasprintf_x.c (new)
     + * WRAPPERS
     + **********************/
     +int
    -+__wrap_asprintf(char **restrict p, const char *restrict fmt, ...)
    ++__wrap_vasprintf(char **restrict p, const char *restrict fmt, va_list ap)
     +{
     +  return -1;
     +}
2:  d3ad213b = 2:  e866897b tests/unit/test_xasprintf.c: Test non-error path of xasprintf()

alejandro-colomar · 2023-10-18T13:14:13Z

Weee!

cryptomilk · 2023-10-18T13:14:17Z

I would not suggest to wrap exit or other real low level functions, it is not really worth testing. Wrap asprintf() or whatever function your implementation calls and mock it, then you can either return NULL or a valid string.

alejandro-colomar · 2023-10-18T13:18:26Z

@cryptomilk

These are the functions I'm trying to test:

./lib/sprintf.h:40:
inline int
xvasprintf(char **restrict s, const char *restrict fmt, va_list ap)
{
	int  len;

	len = vasprintf(s, fmt, ap);
	if (len == -1) {
		perror("asprintf");
		exit(EXIT_FAILURE);
	}

	return len;
}

./lib/sprintf.h:26:
inline int
xasprintf(char **restrict s, const char *restrict fmt, ...)
{
	int      len;
	va_list  ap;

	va_start(ap, fmt);
	len = xvasprintf(s, fmt, ap);
	va_end(ap);

	return len;
}

One test is just checking that it normally behaves as creating a formatted string; that's easy, and we don't need to wrap anything.

But the other test is that the function actually exit(3)s on failure. How would you test that path? We need to actually call exit(3) (or rather, its wrapper). Otherwise, we cannot test that path.

I.e., we want to specifically test this path (which is the main purpose of this function):

	if (len == -1) {
		perror("asprintf");
		exit(EXIT_FAILURE);
	}

It seems interposing vasprintf(3) to make it return -1, and then exit(3) to perform a longjmp(3) is working.

cryptomilk · 2023-10-18T13:27:35Z

Looks fine for me :-)

alejandro-colomar · 2023-10-18T15:36:13Z

Now that these changes work, I've merged them to the main PR: #793

Link: <shadow-maint#816> Acked-by: Andreas Schneider <https://github.com/cryptomilk> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Link: <shadow-maint#816> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Link: <shadow-maint#816> Suggested-by: Iker Pedrosa <ipedrosa@redhat.com> Acked-by: Andreas Schneider <https://github.com/cryptomilk> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Link: <shadow-maint#816> Suggested-by: Iker Pedrosa <ipedrosa@redhat.com> Acked-by: Andreas Schneider <https://github.com/cryptomilk> Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Link: <#816> Suggested-by: Iker Pedrosa <ipedrosa@redhat.com> Acked-by: Andreas Schneider <https://github.com/cryptomilk> Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar added 8 commits October 5, 2023 15:29

lib/copydir.c: Invert conditional to reduce nesting

2d3e668

Signed-off-by: Alejandro Colomar <alx@kernel.org>

lib/sprintf.[ch]: Add x[v]asprintf()

8762538

As other x...() wrappers around functions that allocate, these wrappers are like [v]asprintf(3), but exit on failure. Signed-off-by: Alejandro Colomar <alx@kernel.org>

lib/copydir.c: Use goto to reduce a conditional branch

c5f53d2

Signed-off-by: Alejandro Colomar <alx@kernel.org>

lib/, src/: Use xasprintf() instead of its pattern

2cf69f7

Signed-off-by: Alejandro Colomar <alx@kernel.org>

lib/: Add missing #include <config.h>

0333cdf

Signed-off-by: Alejandro Colomar <alx@kernel.org>

autogen.sh: Prepare CFLAGS before ./configure

4b4c7e9

Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar force-pushed the xasprintf_test branch from 6caaec1 to 7f75d70 Compare October 6, 2023 15:53

alejandro-colomar commented Oct 6, 2023

View reviewed changes

Comment thread tests/unit/test_xasprintf_x.c Outdated

alejandro-colomar force-pushed the xasprintf_test branch 5 times, most recently from 0767c28 to 7f98c6b Compare October 9, 2023 21:58

ikerexxe reviewed Oct 18, 2023

View reviewed changes

Comment thread tests/unit/test_xasprintf_x.c Outdated

alejandro-colomar force-pushed the xasprintf_test branch from 7f98c6b to 0cef708 Compare October 18, 2023 12:00

alejandro-colomar marked this pull request as ready for review October 18, 2023 12:01

alejandro-colomar force-pushed the xasprintf_test branch from 0cef708 to dfaa64f Compare October 18, 2023 12:06

alejandro-colomar force-pushed the xasprintf_test branch from dfaa64f to c23eb2b Compare October 18, 2023 12:16

alejandro-colomar force-pushed the xasprintf_test branch from c23eb2b to 52116fe Compare October 18, 2023 12:19

alejandro-colomar force-pushed the xasprintf_test branch 2 times, most recently from dfbd529 to b03aa7e Compare October 18, 2023 12:34

alejandro-colomar force-pushed the xasprintf_test branch from b03aa7e to d3ad213 Compare October 18, 2023 13:03

alejandro-colomar added 2 commits October 18, 2023 15:12

tests/unit/test_xasprintf_x.c: Test error path of xasprintf()

74d6825

Signed-off-by: Alejandro Colomar <alx@kernel.org>

tests/unit/test_xasprintf.c: Test non-error path of xasprintf()

e866897

Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar force-pushed the xasprintf_test branch from d3ad213 to e866897 Compare October 18, 2023 13:12

alejandro-colomar mentioned this pull request Oct 18, 2023

[x]asprintf(): The axe is still sharp. #793

Merged

alejandro-colomar closed this Oct 18, 2023

alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Oct 19, 2023

tests/unit/test_xasprintf.c: Test non-error path of xasprintf()

cc9f798

Link: <shadow-maint#816> Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar deleted the xasprintf_test branch October 20, 2023 11:07

Conversation

alejandro-colomar commented Oct 6, 2023

Uh oh!

alejandro-colomar commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikerexxe commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023

Uh oh!

cryptomilk commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alejandro-colomar commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cryptomilk commented Oct 18, 2023

Uh oh!

alejandro-colomar commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alejandro-colomar commented Oct 6, 2023 •

edited

Loading

alejandro-colomar commented Oct 18, 2023 •

edited

Loading

alejandro-colomar commented Oct 18, 2023 •

edited

Loading

cryptomilk commented Oct 18, 2023 •

edited

Loading

alejandro-colomar commented Oct 18, 2023 •

edited

Loading

alejandro-colomar commented Oct 18, 2023 •

edited

Loading