AddressSanitizer: negative-size-param in decompose_paths

[l2dy]

Describe the bug yash crashes on start when built with AddressSanitizer.

With the following debug patch applied, variable n passed to wcsnrtombs() is -1 in the last round of decomposing PATH, resulting in a crash in AddressSanitizer.

diff --git a/strbuf.c b/strbuf.c
index 9666b11c..b866e724 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -191,6 +191,7 @@ wchar_t *sb_wcsncat(xstrbuf_T *restrict buf,
 #if HAVE_WCSNRTOMBS
     for (;;) {
         const wchar_t *saves = s;
+        xprintf("sb_wcsncat: str=%ls, nwc=%ld, len=%ld\n", s, n, buf->maxlength - buf->length);
         size_t count = wcsnrtombs(&buf->contents[buf->length],
                 (const wchar_t **) &s, n,
                 buf->maxlength - buf->length, ps);
@@ -210,6 +211,7 @@ wchar_t *sb_wcsncat(xstrbuf_T *restrict buf,
         }
         sb_ensuremax(buf, add(buf->maxlength, MB_CUR_MAX));
     }
+    xprintf("sb_wcsncat: done, str=%ls\n", s);
     assert(buf->contents[buf->length] == '\0');
     return (wchar_t *) s;
 #else

Output:

sb_wcsncat: str=/usr/bin:/bin:/usr/sbin:/sbin, nwc=8, len=15
sb_wcsncat: done, str=(null)
sb_wcsncat: str=/bin:/usr/sbin:/sbin, nwc=4, len=15
sb_wcsncat: done, str=(null)
sb_wcsncat: str=/usr/sbin:/sbin, nwc=9, len=15
sb_wcsncat: done, str=(null)
sb_wcsncat: str=/sbin, nwc=-1, len=15

To Reproduce Steps to reproduce the behavior:

1. Run ./configure without -fsanitize=address in CFLAGS and LDFLAGS
2. Modify Makefile by hand, adding -fsanitize=address to both flags
3. make yash
4. PATH="/usr/bin:/bin:/usr/sbin:/sbin" ./yash
5. See error
6. Apply debug patch and try again
7. See nwc=-1

Expected behavior Yash does not crash with AddressSanitizer.

Screenshots (without patch)

./yash
yash(17524,0x7ff85e79ab40) malloc: nano zone abandoned due to inability to reserve vm space.
=================================================================
==17524==ERROR: AddressSanitizer: negative-size-param: (size=-1)
    #0 0x10196877b in wcsnrtombs+0x50b (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x7d77b)
    #1 0x100e42082 in sb_wcsncat strbuf.c:194
    #2 0x100e44e0a in malloc_wcsntombs strbuf.c:513
    #3 0x100e4bc29 in malloc_wcstombs strbuf.h:448
    #4 0x100e492d7 in decompose_paths variable.c:1153
    #5 0x100e48cf0 in init_environment variable.c:278
    #6 0x100e70aaa in main yash.c:144
    #7 0x7ff81ae103a5 in start+0x795 (dyld:x86_64+0xfffffffffff5c3a5)

0x61d000001c54 is located 2004 bytes inside of 2080-byte region [0x61d000001480,0x61d000001ca0)
allocated by thread T0 here:
    #0 0x1019c9400 in malloc+0xa0 (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0xde400)
    #1 0x7ff81b0a1a2f in wcsdup+0x1e (libsystem_c.dylib:x86_64+0x69a2f)
    #2 0x10199ce2f in wcsdup+0xcf (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0xb1e2f)
    #3 0x100e48985 in init_environment variable.c:267
    #4 0x100e70aaa in main yash.c:144
    #5 0x7ff81ae103a5 in start+0x795 (dyld:x86_64+0xfffffffffff5c3a5)

SUMMARY: AddressSanitizer: negative-size-param (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x7d77b) in wcsnrtombs+0x50b
==17524==ABORTING

Environment (please complete the following information):

• OS: macOS
• Version commit bd1fd3f1420569ce09551cfc6a8acfb9092fd6dc
• Locale en_US.UTF-8
• Built with clang version 17.0.5 and -g -O0 -fsanitize=address

Additional context N/A

[magicant]

Thanks for the report, but I have difficulty interpreting the message from the address sanitizer. The third parameter to the wcsnrtombs function is of type size_t, which is an unsigned integer and can't be negative. The parameter specifies the maximum number of characters to be converted, which is irrelevant to the actual size of arrays involved. Maybe the sanitizer is incorrectly configured to perform some check about this parameter?

PS: To print a size_t value, you should be using %zu rather than %ld.

[l2dy]

You are right. After further investigation, this seems to be a bug in AddressSanitizer. https://github.com/google/sanitizers/issues/1524

However, this makes me wonder if wcsrtombs should be used instead when size is not specified. We don't have a buffer as big as Size_max anyway, and size_t len is still there to prevent buffer overflow. Why use such a large number here?

[magicant]

Defining sb_wcscat as a special case of sb_wcsncat is much simpler than defining them as separate functions that are very similar but differ only in one line that calls either wcsrtombs or wcsnrtombs.

sb_wcscat is defined as: https://github.com/magicant/yash/blob/2e02670ad531ea8ac445217ce70d32915c736709/strbuf.h#L322C1-L328

[l2dy]

Defining sb_wcscat as a special case of sb_wcsncat is much simpler than defining them as separate functions that are very similar but differ only in one line that calls either wcsrtombs or wcsnrtombs.

Yes, but we already have sb_wcscat and malloc_wcstombs for systems without wcsnrtombs anyway. Could they be enabled on all systems? https://github.com/magicant/yash/blob/2e02670ad531ea8ac445217ce70d32915c736709/strbuf.c#L233-L236

[magicant]

Hmm, I don't want to bloat the binary just to make the misbehaving analyzer happy.

[l2dy]

Sure. Closing this issue then. Thanks for taking a look!