From 0c5c2632dcf85c380bb88eae83825c63a87141de Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 12 Apr 2021 13:23:39 -0400 Subject: [PATCH] Fix: utils: avoid strncpy overlap in utils_partial_realpath MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit When running the test_utils_expand_path test with ASan enabled, I get: ➜ lttng-tools ./tests/unit/test_utils_expand_path 1..29 INPUT: /a/b/c/d/e ================================================================= ==1485873==ERROR: AddressSanitizer: strncpy-param-overlap: memory ranges [0x621000021d00,0x621000021d0b) and [0x621000021d00, 0x621000021d0b) overlap #0 0x7ffff761fd97 in __interceptor_strncpy /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cpp:481 #1 0x555555573834 in utils_partial_realpath /home/simark/src/lttng-tools/src/common/utils.c:195 #2 0x55555557410b in _utils_expand_path /home/simark/src/lttng-tools/src/common/utils.c:374 #3 0x555555574340 in utils_expand_path /home/simark/src/lttng-tools/src/common/utils.c:420 #4 0x555555570b28 in test_utils_expand_path /home/simark/src/lttng-tools/tests/unit/test_utils_expand_path.c:274 #5 0x55555557119e in main /home/simark/src/lttng-tools/tests/unit/test_utils_expand_path.c:345 #6 0x7ffff725fb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) #7 0x55555556fa3d in _start (/home/simark/build/lttng-tools/tests/unit/test_utils_expand_path+0x1ba3d) 0x621000021d00 is located 0 bytes inside of 4096-byte region [0x621000021d00,0x621000022d00) allocated by thread T0 here: #0 0x7ffff7677639 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x55555557269d in zmalloc /home/simark/src/lttng-tools/src/common/macros.h:45 #2 0x555555573d34 in _utils_expand_path /home/simark/src/lttng-tools/src/common/utils.c:335 #3 0x555555574340 in utils_expand_path /home/simark/src/lttng-tools/src/common/utils.c:420 #4 0x555555570b28 in test_utils_expand_path /home/simark/src/lttng-tools/tests/unit/test_utils_expand_path.c:274 #5 0x55555557119e in main /home/simark/src/lttng-tools/tests/unit/test_utils_expand_path.c:345 #6 0x7ffff725fb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) 0x621000021d00 is located 0 bytes inside of 4096-byte region [0x621000021d00,0x621000022d00) allocated by thread T0 here: #0 0x7ffff7677639 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x55555557269d in zmalloc /home/simark/src/lttng-tools/src/common/macros.h:45 #2 0x555555573d34 in _utils_expand_path /home/simark/src/lttng-tools/src/common/utils.c:335 #3 0x555555574340 in utils_expand_path /home/simark/src/lttng-tools/src/common/utils.c:420 #4 0x555555570b28 in test_utils_expand_path /home/simark/src/lttng-tools/tests/unit/test_utils_expand_path.c:274 #5 0x55555557119e in main /home/simark/src/lttng-tools/tests/unit/test_utils_expand_path.c:345 #6 0x7ffff725fb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) The sole caller of utils_partial_realpath, _utils_expand_path, passes the same buffer (resolved_path) for the input and output. This causes utils_partial_realpath to call strncpy with overlapping strings. Fix it by making utils_partial_realpath allocate new memory for the returned string itself. This causes one more allocation than the current code, because we don't re-use the existing buffer, but this should be fine since this isn't exactly performance-critical code. I think the code is easier to follow as a result. Signed-off-by: Simon Marchi Signed-off-by: Jérémie Galarneau Change-Id: Iab983e2f44fa57563b11ac6e9c03a41150669d9e --- src/common/utils.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/common/utils.c b/src/common/utils.c index 4827cfc7b..76185c11e 100644 --- a/src/common/utils.c +++ b/src/common/utils.c @@ -60,16 +60,15 @@ * but the /tmp/test1 does, the real path for /tmp/test1 is concatened with * /test2/test3 then returned. In normal time, realpath(3) fails if the end * point directory does not exist. - * In case resolved_path is NULL, the string returned was allocated in the - * function and thus need to be freed by the caller. The size argument allows - * to specify the size of the resolved_path argument if given, or the size to - * allocate. + * + * Return a newly-allocated string. */ static -char *utils_partial_realpath(const char *path, char *resolved_path, size_t size) +char *utils_partial_realpath(const char *path) { char *cut_path = NULL, *try_path = NULL, *try_path_prev = NULL; const char *next, *prev, *end; + char *resolved_path = NULL; /* Safety net */ if (path == NULL) { @@ -150,13 +149,11 @@ char *utils_partial_realpath(const char *path, char *resolved_path, size_t size) cut_path = NULL; } - /* Allocate memory for the resolved path if necessary */ + /* Allocate memory for the resolved path. */ + resolved_path = zmalloc(LTTNG_PATH_MAX); if (resolved_path == NULL) { - resolved_path = zmalloc(size); - if (resolved_path == NULL) { - PERROR("zmalloc resolved path"); - goto error; - } + PERROR("zmalloc resolved path"); + goto error; } /* @@ -180,7 +177,8 @@ char *utils_partial_realpath(const char *path, char *resolved_path, size_t size) } /* Concatenate the strings */ - snprintf(resolved_path, size, "%s%s", try_path_prev, cut_path); + snprintf(resolved_path, LTTNG_PATH_MAX, "%s%s", + try_path_prev, cut_path); /* Free the allocated memory */ free(cut_path); @@ -192,7 +190,7 @@ char *utils_partial_realpath(const char *path, char *resolved_path, size_t size) * return it as is */ } else { - strncpy(resolved_path, path, size); + strncpy(resolved_path, path, LTTNG_PATH_MAX); } /* Then we return the 'partially' resolved path */ @@ -371,11 +369,13 @@ char *_utils_expand_path(const char *path, bool keep_symlink) if (keep_symlink) { /* Resolve partially our path */ - absolute_path = utils_partial_realpath(absolute_path, - absolute_path, LTTNG_PATH_MAX); - if (!absolute_path) { + char *new_absolute_path = utils_partial_realpath(absolute_path); + if (!new_absolute_path) { goto error; } + + free(absolute_path); + absolute_path = new_absolute_path; } ret = expand_double_slashes_dot_and_dotdot(absolute_path); -- 2.34.1