Safe RE_VA_ARG helpers #758

sreimers · 2023-04-10T08:03:56Z

RE_VA_ARG helpers ensure type size safety by using the _Generic keyword (C11 only) to determine the size of the argument based on its type. This ensures that only arguments of the expected types are passed to the function, and prevents type mismatches that could lead to undefined behavior or security vulnerabilities.

maximilianfridrich · 2023-04-12T07:18:14Z

Very impressive, thank you! I am still spending some time in that area too and have written an LLVM Pass to help me find possible errors in variadic functions in general (there are many variadic functions), not only format string related ones.

I am basically done going through the whole code base, there is one more call of a variadic function which I am fixing that is non-trivial. I will open a PR for that soon and will report my findings.

alfredh · 2023-04-29T08:26:37Z

test/test.c

@@ -515,7 +515,7 @@ static int test_unit(const char *name, bool verbose)
 		for (i=0; i<RE_ARRAY_SIZE(tests); i++) {

 			if (verbose) {
-				re_printf("test %u -- %s\n",
+				re_printf("test %zu -- %s\n",


some of the hunks in this patch are fixing the formatting strings.

it could be an idea to make a separate PR for these fixes ...

alfredh · 2023-04-29T08:27:38Z

this is great work!

Variable arguments is certainly a source for errors and security issues :)

alfredh · 2023-05-03T13:09:20Z

I think this can be merged now..

You can also rebase it with main if you want ...

sreimers · 2023-05-05T13:05:53Z

I think this can be merged now..

Currently I have two open issues/todos:

Fixing win32 x86 assert failure
Create baresip PR to handle src/config.c (has many ARGS and uses #ifdef between them)

alfredh · 2023-05-15T07:18:04Z

this patch is quite large, around 900 lines.

since the patch is quite large and intrusive, would it be possible to split it up into smaller parts ?

alfredh · 2023-06-17T09:42:38Z

baresip/baresip#2621

sreimers · 2023-06-17T11:34:46Z

Nice thanks! I will rebase this PR soon.

alfredh · 2023-06-19T12:38:57Z

baresip/baresip#2627

alfredh · 2023-06-24T13:25:43Z

include/re_types.h

+	if ((safe)) {                                                         \
+		size_t sz = va_arg((ap), size_t);                             \
+		assert(sz && "RE_VA_ARG: no more arguments");                 \
+		assert(sz <= sizeof(type) &&                                  \


Should we consider a warning instead of assert ?

It would be nice, if a test fails if size is mismatched or no more args available.
btw: cmake sets -DNDEBUG, on CMAKE_BUILD_TYPE=Release builds and assert() is ignored.

Another approach is to return an error code if the format is wrong.

This means that e.g. mbuf_printf would return the error code.

Another approach is to return an error code if the format is wrong.

Most printf code ignores the return code I think, but its already set and returned here:

if (!sz || sz > sizeof(type)) { \ err = EINVAL; \ goto out; \ }

With Release build type assert is not called (I can make this more visible with #ifndef RELEASE)

src/fmt/time.c

src/mbuf/mbuf.c

alfredh · 2023-07-01T10:41:14Z

The test "dtls_turn" is a combo test, I am not sure if its needed anymore.
All tests in retest are testing single modules, except "combo".

&& ./retest.exe -v -ri
regular tests:       Assertion failed: sz <= sizeof(unsigned long) && "RE_VA_ARG: arg is not compatible", file /home/runner/work/re/re/baresip-win32/re/src/fmt/print.c, line 349
Error: Signal (22) 0: test_dtls_turn (:0)
1: test_dtls_turn (:0)
2: raise (:0)
3: abort (:0)
4: assert (:0)
5: test_dtls_turn (:0)
6: test_dtls_turn (:0)
7: test_dtls_turn (:0)
8: test_dtls_turn (:0)
9: test_dtls_turn (:0)

Can you also log the printf string that is being printed ?

alfredh · 2023-07-03T15:24:01Z

I think you should try this patch on both 32-bits and 64-bits platforms.
For example size_t is 32-bits for 32-bits platforms.

Before the %zu fix in http, the test was failing on Mingw:

test 50 -- test_h265_packet
test 51 -- test_hash
test 52 -- test_hmac_sha1
test 53 -- test_hmac_sha256
test 54 -- test_http
test 55 -- test_http_loop
test 56 -- test_http_large_body
test 57 -- test_http_conn
... http_reqconn_send 639
Assertion failed: sz <= sizeof(unsigned long) && "RE_VA_ARG: arg is not compatible", file /home/alfredh/git/baresip-win32/re/s
rc/fmt/print.c, line 349

Error: Signal (22) 0:  (:0)
1:  (:0)
2: raise (:0)
3: _wassert (:0)
4: _assert (:0)
5: _assert (:0)
6: _assert (:0)
7: _assert (:0)
8: _assert (:0)
9: _assert (:0)

make: *** [Makefile:131: test] Error 3

perhaps you can rebase this PR to main HEAD ?

juha-h · 2023-08-13T15:52:25Z

Looks like after this commit, baresip-studio project armeabi-v7a build stopped working:

/opt/Android/ndk/25.2.9519653/toolchains/llvm/prebuilt/linux-x86_64/bin/clang --target=armv7-none-linux-androideabi23 --sysroot=/opt/Android/ndk/25.2.9519653/toolchains/llvm/prebuilt/linux-x86_64/sysroot -DARCH=\"armv7-a\" -DHAVE_ACCEPT4 -DHAVE_ARC4RANDOM -DHAVE_ATOMIC -DHAVE_EPOLL -DHAVE_FORK -DHAVE_GETOPT -DHAVE_INET6 -DHAVE_PRCTL -DHAVE_PTHREAD -DHAVE_PWD_H -DHAVE_ROUTE_LIST -DHAVE_SELECT -DHAVE_SELECT_H -DHAVE_SETRLIMIT -DHAVE_SIGNAL -DHAVE_STRERROR_R -DHAVE_STRINGS_H -DHAVE_SYSLOG -DHAVE_SYS_TIME_H -DHAVE_UNAME -DHAVE_UNISTD_H -DHAVE_UNIXSOCK=1 -DOS=\"Android\" -DUSE_ZLIB -Dbaresip_EXPORTS -I/usr/src/baresip-studio/app/src/main/cpp/../../../../distribution/openssl/include -I/usr/src/baresip-studio/app/src/main/cpp/../../../../distribution/re/include -I/usr/src/baresip-studio/app/src/main/cpp/../../../../distribution/baresip/include -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -march=armv7-a -mthumb -Wformat -Werror=format-security -DHAVE_INTTYPES_H -lstdc++ -fno-limit-debug-info  -fPIC -MD -MT CMakeFiles/baresip.dir/baresip.c.o -MF CMakeFiles/baresip.dir/baresip.c.o.d -o CMakeFiles/baresip.dir/baresip.c.o -c /usr/src/baresip-studio/app/src/main/cpp/baresip.c

/usr/src/baresip-studio/app/src/main/cpp/baresip.c:41:9: error: cannot combine with previous 'long long' declaration specifier
    l = re_snprintf(&(debug_buf[0]), 2047, "%H", net_debug, baresip_network());
        ^
/usr/src/baresip-studio/app/src/main/cpp/../../../../distribution/re/include/re_fmt.h:117:39: note: expanded from macro 're_snprintf'
        _re_snprintf_s((str), (size), (fmt), RE_VA_ARGS(__VA_ARGS__))
                                             ^
/usr/src/baresip-studio/app/src/main/cpp/../../../../distribution/re/include/re_types.h:363:25: note: expanded from macro 'RE_VA_ARGS'
#define RE_VA_ARGS(...) RE_ARG_N2(RE_ARG_VA_NUM(__VA_ARGS__), __VA_ARGS__)
                        ^
/usr/src/baresip-studio/app/src/main/cpp/../../../../distribution/re/include/re_types.h:362:27: note: expanded from macro 'RE_ARG_N2'
#define RE_ARG_N2(N, ...) RE_ARG_N3(N, __VA_ARGS__)
                          ^
note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
/usr/src/baresip-studio/app/src/main/cpp/../../../../distribution/re/include/re_types.h:338:29: note: expanded from macro 'RE_ARG_2'
#define RE_ARG_2(expr, ...) RE_ARG_SIZE(expr), (expr), RE_ARG_1(__VA_ARGS__)
                            ^
/usr/src/baresip-studio/app/src/main/cpp/../../../../distribution/re/include/re_types.h:325:7: note: expanded from macro 'RE_ARG_SIZE'
        long long:              sizeof(long long),                            \
             ^
/usr/src/baresip-studio/app/src/main/cpp/baresip.c:11:18: note: expanded from macro 'long'
    #define long long long
                 ^

Any suggestions how to fix the build or should I just stop supporting 32 bit armeabi-v7a architecture?

juha-h · 2023-08-13T15:59:44Z

The build worked when I commented out this:

//#if defined(__ARM_ARCH_7A__)
//    #define long long long
//#endif

But I don't have any armeabi-v7a devices to test if the app actually works.

sreimers · 2023-08-13T16:31:54Z

#define long long long

Where is this defined?

juha-h · 2023-08-13T16:51:46Z

Sebastian Reimers writes:

> #define long long long Where is this defined?

It was defined in my app and was needed before this commit. Otherwise 32 bit build didn't work.

sreimers force-pushed the re_va_arg_size branch from 0754e1a to 76bacc4 Compare April 10, 2023 09:54

sreimers changed the title ~~Re va arg size~~ RE_VA_ARG helpers Apr 10, 2023

sreimers added this to the v3.2.0 milestone Apr 10, 2023

sreimers force-pushed the re_va_arg_size branch 2 times, most recently from dd7e64c to 3c75dd7 Compare April 12, 2023 06:54

sreimers changed the title ~~RE_VA_ARG helpers~~ Safe RE_VA_ARG helpers Apr 14, 2023

sreimers marked this pull request as ready for review April 14, 2023 21:03

sreimers force-pushed the re_va_arg_size branch from cc3ed0f to 034c907 Compare April 28, 2023 08:12

alfredh reviewed Apr 29, 2023

View reviewed changes

sreimers force-pushed the re_va_arg_size branch from 034c907 to a608add Compare May 2, 2023 07:17

alfredh mentioned this pull request May 3, 2023

test: fix printf formating #783

Merged

alfredh removed this from the v3.2.0 milestone May 18, 2023

sreimers added this to the v3.3.0 milestone May 19, 2023

alfredh removed this from the v3.3.0 milestone Jun 8, 2023

sreimers force-pushed the re_va_arg_size branch from a608add to b24aa98 Compare June 19, 2023 08:51

alfredh reviewed Jun 24, 2023

View reviewed changes

alfredh reviewed Jul 1, 2023

View reviewed changes

src/fmt/time.c Outdated Show resolved Hide resolved

alfredh reviewed Jul 1, 2023

View reviewed changes

src/mbuf/mbuf.c Outdated Show resolved Hide resolved

alfredh mentioned this pull request Jul 3, 2023

http: use correct formatting %zu for size_t #866

Merged

sreimers force-pushed the re_va_arg_size branch from b24aa98 to 67dae1d Compare July 3, 2023 15:29

sreimers added 22 commits August 10, 2023 15:00

types: add VA_ARG size helpers

ad831c4

RE_VA_ARG work in progress

ee8e9f3

use VA_OPT and ignore clang gnu-zero-variadic-macro-arguments warnings

210268a

use old variant and fix linting

313a714

add HAVE_RE_ARG

59dd9f1

fix cmakelint

6df9ea2

try va_list pointer with copy

41fd5cb

enable RE_VA_ARG

31748fe

refactoring

6df01e5

remove unecessary __VA_ARGS__

ab19313

add DISABLE_RE_ARG, try win32 and exclude release builds

3e077e1

every va_copy needs its own va_end

b3f7f27

refactor arg handling (fixes unused sz with release builds)

3493eee

exclude strict ansi

d745197

cleanup and add __VA_OPT__ C23 hint

aa542e5

refactoring

16676b3

enable for RELEASE and cleanup

d14a149

type: add re_assert definition

86a8566

try to optimize RE_VA_ARG helper

5624088

use EOVERFLOW instead of EINVAL

4b4d71d

remove unneded include assert.h

ae66843

remove newline

f7029f6

sreimers force-pushed the re_va_arg_size branch from 1fc6780 to f7029f6 Compare August 10, 2023 13:01

improve endif comment

b5a9b5d

sreimers merged commit aedaf42 into main Aug 10, 2023
38 checks passed

sreimers deleted the re_va_arg_size branch August 10, 2023 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safe RE_VA_ARG helpers #758

Safe RE_VA_ARG helpers #758

sreimers commented Apr 10, 2023 •

edited

Loading

maximilianfridrich commented Apr 12, 2023 •

edited

Loading

alfredh Apr 29, 2023

alfredh commented Apr 29, 2023

alfredh commented May 3, 2023

sreimers commented May 5, 2023

alfredh commented May 15, 2023

alfredh commented Jun 17, 2023

sreimers commented Jun 17, 2023

alfredh commented Jun 19, 2023

alfredh Jun 24, 2023

sreimers Jun 24, 2023 •

edited

Loading

alfredh Jul 15, 2023

sreimers Aug 9, 2023 •

edited

Loading

alfredh commented Jul 1, 2023

alfredh commented Jul 3, 2023

juha-h commented Aug 13, 2023 •

edited

Loading

juha-h commented Aug 13, 2023

sreimers commented Aug 13, 2023

juha-h commented Aug 13, 2023 via email

Safe RE_VA_ARG helpers #758

Safe RE_VA_ARG helpers #758

Conversation

sreimers commented Apr 10, 2023 • edited Loading

maximilianfridrich commented Apr 12, 2023 • edited Loading

alfredh Apr 29, 2023

Choose a reason for hiding this comment

alfredh commented Apr 29, 2023

alfredh commented May 3, 2023

sreimers commented May 5, 2023

alfredh commented May 15, 2023

alfredh commented Jun 17, 2023

sreimers commented Jun 17, 2023

alfredh commented Jun 19, 2023

alfredh Jun 24, 2023

Choose a reason for hiding this comment

sreimers Jun 24, 2023 • edited Loading

Choose a reason for hiding this comment

alfredh Jul 15, 2023

Choose a reason for hiding this comment

sreimers Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

alfredh commented Jul 1, 2023

alfredh commented Jul 3, 2023

juha-h commented Aug 13, 2023 • edited Loading

juha-h commented Aug 13, 2023

sreimers commented Aug 13, 2023

juha-h commented Aug 13, 2023 via email

sreimers commented Apr 10, 2023 •

edited

Loading

maximilianfridrich commented Apr 12, 2023 •

edited

Loading

sreimers Jun 24, 2023 •

edited

Loading

sreimers Aug 9, 2023 •

edited

Loading

juha-h commented Aug 13, 2023 •

edited

Loading