From: Simon Marchi <simon.marchi@efficios.com>
Date: Thu, 5 Aug 2021 01:24:47 +0000 (-0400)
Subject: Fix unknown option name length computation in set_error
X-Git-Url: http://git.efficios.com/?p=argpar.git;a=commitdiff_plain;h=c530e1dcf020f4f6189ebafc5fd6cb99d0f013d4

Fix unknown option name length computation in set_error

When running the tests with AddressSanitizer enabled, we get:

    ==279682==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000001891 at pc 0x7fefdfa7ae6f bp 0x7ffd66b45560 sp 0x7ffd66b44d08
    WRITE of size 2 at 0x602000001891 thread T0
	#0 0x7fefdfa7ae6e in __interceptor_memcpy /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
	#1 0x55e7163783d8 in set_error (/home/simark/build/argpar/tests/test_argpar+0xe3d8)
	#2 0x55e716378d66 in parse_short_opt_group (/home/simark/build/argpar/tests/test_argpar+0xed66)
	#3 0x55e71637981c in parse_orig_arg_opt (/home/simark/build/argpar/tests/test_argpar+0xf81c)
	#4 0x55e716379eb1 in argpar_iter_next (/home/simark/build/argpar/tests/test_argpar+0xfeb1)
	#5 0x55e716374065 in test_fail (/home/simark/build/argpar/tests/test_argpar+0xa065)
	#6 0x55e716374c13 in fail_tests (/home/simark/build/argpar/tests/test_argpar+0xac13)
	#7 0x55e71637644d in main (/home/simark/build/argpar/tests/test_argpar+0xc44d)
	#8 0x7fefdf767b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
	#9 0x55e71636f3ad in _start (/home/simark/build/argpar/tests/test_argpar+0x53ad)

    0x602000001891 is located 0 bytes to the right of 1-byte region [0x602000001890,0x602000001891)
    allocated by thread T0 here:
	#0 0x7fefdfaf8459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
	#1 0x55e716378311 in set_error (/home/simark/build/argpar/tests/test_argpar+0xe311)
	#2 0x55e716378d66 in parse_short_opt_group (/home/simark/build/argpar/tests/test_argpar+0xed66)
	#3 0x55e71637981c in parse_orig_arg_opt (/home/simark/build/argpar/tests/test_argpar+0xf81c)
	#4 0x55e716379eb1 in argpar_iter_next (/home/simark/build/argpar/tests/test_argpar+0xfeb1)
	#5 0x55e716374065 in test_fail (/home/simark/build/argpar/tests/test_argpar+0xa065)
	#6 0x55e716374c13 in fail_tests (/home/simark/build/argpar/tests/test_argpar+0xac13)
	#7 0x55e71637644d in main (/home/simark/build/argpar/tests/test_argpar+0xc44d)
	#8 0x7fefdf767b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

This is due to an operator precedence mistake in the modified
expression.

    (gdb) p unknown_opt_name
    $2 = 0x7fffffffd870 "e"
    (gdb) p strlen(unknown_opt_name) + 1 + is_short ? 1 : 2
    $3 = 1

The addition is evaluated before the ternary operator, so we end up
choosing between values 1 and 2.  And we always choose 1, because the
result of the addition is always non-zero.

The intent is in fact to add 1 or 2 to `strlen(unknown_opt_name) + 1`,
depending on whether the short option is used.  Add parenthesis around
the ternary to fix it.

Note that clang is cool enough to provide a diagnostic about this:

    /home/simark/src/argpar/argpar/argpar.c:291:44: error: operator '?:' has lower precedence than '+'; '+' will be evaluated first [-Werror,-Wparentheses]
			    strlen(unknown_opt_name) + 1 + is_short ? 1 : 2);
			    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
    /home/simark/src/argpar/argpar/argpar.c:21:21: note: expanded from macro 'ARGPAR_CALLOC'
	    ((_type *) calloc((_nmemb), sizeof(_type)))
			       ^~~~~~
    /home/simark/src/argpar/argpar/argpar.c:291:44: note: place parentheses around the '+' expression to silence this warning
			    strlen(unknown_opt_name) + 1 + is_short ? 1 : 2);
			    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
    /home/simark/src/argpar/argpar/argpar.c:21:21: note: expanded from macro 'ARGPAR_CALLOC'
	    ((_type *) calloc((_nmemb), sizeof(_type)))
			       ^~~~~~
    /home/simark/src/argpar/argpar/argpar.c:291:44: note: place parentheses around the '?:' expression to evaluate it first
			    strlen(unknown_opt_name) + 1 + is_short ? 1 : 2);
							   ~~~~~~~~~^~~~~~~
    /home/simark/src/argpar/argpar/argpar.c:21:21: note: expanded from macro 'ARGPAR_CALLOC'
	    ((_type *) calloc((_nmemb), sizeof(_type)))
			       ^~~~~~

Change-Id: Ibf3a4ac46c16c42a8279ede5f610e7c8797c9aed
Signed-off-by: Simon Marchi <simon.marchi@efficios.com>
---

diff --git a/argpar/argpar.c b/argpar/argpar.c
index 47056b8..1c3a061 100644
--- a/argpar/argpar.c
+++ b/argpar/argpar.c
@@ -288,7 +288,7 @@ int set_error(struct argpar_error ** const error,
 
 	if (unknown_opt_name) {
 		(*error)->unknown_opt_name = ARGPAR_CALLOC(char,
-			strlen(unknown_opt_name) + 1 + is_short ? 1 : 2);
+			strlen(unknown_opt_name) + 1 + (is_short ? 1 : 2));
 		if (!(*error)->unknown_opt_name) {
 			goto error;
 		}