[PATCH 0/1] MR8071: ucrtbase/tests: Avoid reading behind buffer in test_swprintf (ASan).

[Bernhard Übelacker]

The test `test_swprintf` creates an un-terminated buffer and calls `WideCharToMultiByte` without explicit srclen on it.

This patch wraps the buffer into a struct providing a termination without modifying the test itself.

<details>
<summary>ASan output:</summary>

```
=================================================================
==ucrtbase_test.exe==532==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffffe1ffac0 at pc 0x6ffffe845638 bp 0x7ffffe1ff030 sp 0x7ffffe1ff078
READ of size 24 at 0x7ffffe1ffac0 thread T0
    #0 0x6ffffe845637 in wcslen C:/llvm-mingw/llvm-mingw/llvm-project/compiler-rt/lib/asan/../sanitizer_common\sanitizer_common_interceptors.inc:7215:3
    #1 0x6fffff7d1eff in lstrlenW .../wine/include/winbase.h:2574:5
    #2 0x6fffff7d2a33 in WideCharToMultiByte .../wine/dlls/kernelbase/locale.c:7382:30
    #3 0x000140016e6c in test_swprintf .../wine/dlls/ucrtbase/tests/printf.c:196:9
    #4 0x000140015f3e in func_printf .../wine/dlls/ucrtbase/tests/printf.c:891:5
    #5 0x00014002da0e in run_test .../wine/include/wine/test.h:765:5
    #6 0x00014002d53a in main .../wine/include/wine/test.h:884:12
    #7 0x00014002f005 in mainCRTStartup .../wine/dlls/msvcrt/crt_main.c:58:11
    #8 0x6fffffc355be in BaseThreadInitThunk .../wine/dlls/kernel32/thread.c:61:24
    #9 0x6fffffdbbcc2 in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x17004bcc2)

Address 0x7ffffe1ffac0 is located in stack of thread T0 at offset 256 in frame
    #0 0x000140016b6f in test_swprintf .../wine/dlls/ucrtbase/tests/printf.c:176

  This frame has 8 object(s):
    [32, 44) 'str_short' (line 177)
    [64, 80) 'str_justfit' (line 178)
    [96, 114) 'str_justfits' (line 179)
    [160, 182) 'str_muchlonger' (line 180)
    [224, 226) 'str_empty' (line 181)
    [240, 256) 'buffer' (line 184)
    [272, 280) 'narrow' (line 185) <== Memory access at offset 256 partially underflows this variable
    [304, 320) 'narrow_fmt' (line 185)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp, SEH and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow .../wine/include/winbase.h:2574:5 in lstrlenW
Shadow bytes around the buggy address:
  0x7ffffe1ff800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ffffe1ff880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ffffe1ff900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ffffe1ff980: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 04 f2 f2
  0x7ffffe1ffa00: 00 00 f2 f2 00 00 02 f2 f2 f2 f2 f2 00 00 06 f2
=>0x7ffffe1ffa80: f2 f2 f2 f2 02 f2 00 00[f2]f2 00 f2 f2 f2 00 00
  0x7ffffe1ffb00: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ffffe1ffb80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ffffe1ffc00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ffffe1ffc80: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 f8 f8 f8 f8
  0x7ffffe1ffd00: f8 f8 f8 f8 f3 f3 f3 f3 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
```

</details>

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8071

[PATCH 1/1] ucrtbase/tests: Avoid reading behind buffer in test_swprintf (ASan).

[Bernhard Übelacker]

From: Bernhard Übelacker <bernhardu(a)mailbox.org>

---
 dlls/ucrtbase/tests/printf.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/dlls/ucrtbase/tests/printf.c b/dlls/ucrtbase/tests/printf.c
index 75bd7830869..ff2b274c996 100644
--- a/dlls/ucrtbase/tests/printf.c
+++ b/dlls/ucrtbase/tests/printf.c
@@ -181,10 +181,17 @@ static void test_swprintf (void)
     const wchar_t str_empty[]      = {0};
     const wchar_t *tests[] = {str_short, str_justfit, str_justfits, str_muchlonger};
 
-    wchar_t buffer[8];
+    struct {
+        wchar_t buffer[8];
+        wchar_t chk;
+        wchar_t term;
+    } buf;
+#define buffer buf.buffer
     char narrow[8], narrow_fmt[16];
     const int bufsiz = ARRAY_SIZE(buffer);
     unsigned int i;
+    memset(&buf, 0xcd, sizeof(buf));
+    buf.term = L'\0';
 
     /* Legacy _snprintf style termination */
     for (i = 0; i < ARRAY_SIZE(tests); i++) {
@@ -253,6 +260,9 @@ static void test_swprintf (void)
         "Failure to swprintf a zero length string to a zero length buffer\n");
     ok (vswprintf_wrapper (0, buffer, 0, str_empty) == -1,
         "Failure to swprintf a zero length string to a zero length buffer\n");
+
+    ok (buf.chk == 0xcdcd, "buf.chk got overwritten.\n");
+#undef buffer
 }
 
 static int WINAPIV vfprintf_wrapper(FILE *file,
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8071

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/ucrtbase/tests/printf.c:
> const int n        = vswprintf_wrapper (_CRT_INTERNAL_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION, buffer, bufsiz, fmt);
>          const int valid    = n < 0 ? bufsiz : (n == bufsiz ? n : n+1);
>
> WideCharToMultiByte (CP_ACP, 0, buffer, -1, narrow, sizeof(narrow), NULL, NULL);

It makes more sense to fix WideCharToMultiByte call instead (it needs to be done in all the loops):
```suggestion:-0+0
        WideCharToMultiByte (CP_ACP, 0, buffer, valid, narrow, sizeof(narrow), NULL, NULL);
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8071#note_103756