Fix: utils: avoid strncpy overlap in utils_partial_realpath

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 <simon.marchi@efficios.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Change-Id: Iab983e2f44fa57563b11ac6e9c03a41150669d9e

diff --git a/src/common/utils.c b/src/common/utils.c
index 4827cfc7b732947ff8e11cfe1ac8b6781f21e083..76185c11e54ee8439916280ceeca0b1b3897c73e 100644 (file)
--- 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);