[PATCH v5 0/1] MR7047: xmllite/tests: Avoid buffer overflow by using LONG_PTR. (ASan)

[Bernhard Übelacker (＠bernhardu)]

This appeared by running tests with PE-side ASan enabled, just with x86_64.

The enum DtdProcessing seems to have just a size of four bytes, but dereferencing `LONG_PTR *value` writes 8 bytes.

```
=================================================================
==560==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffffe1ff8c0 at pc 0x6ffff7b33f83 bp 0x7ffffe1fed00 sp 0x7ffffe1fed48
WRITE of size 8 at 0x7ffffe1ff8c0 thread T0
05c0:fixme:file:server_get_file_info Unsupported info class e
    #0 0x6ffff7b33f82 in xmlreader_GetProperty .../wine/dlls/xmllite/reader.c:2807:20
    #1 0x000140002254 in IXmlReader_GetProperty .../obj\include\xmllite.h:331:12
    #2 0x000140002254 in test_reader_create .../wine/dlls/xmllite/tests/reader.c:582:10
    #3 0x000140002254 in func_reader .../wine/dlls/xmllite/tests/reader.c:2676:5
    #4 0x00014006bad1 in run_test .../wine/include/wine/test.h:765:5
    #5 0x00014006bad1 in main .../wine/include/wine/test.h:884:12
    #6 0x00014006d94f in mainCRTStartup .../wine/dlls/msvcrt/crt_main.c:58:11
    #7 0x6ffffbdc4808 in BaseThreadInitThunk .../wine/dlls/kernel32\thread.c:61:5
    #8 0x6ffffad1fa1a in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x17000fa1a)

Address 0x7ffffe1ff8c0 is located in stack of thread T0 at offset 2784 in frame
    #0 0x00014000100f in func_reader .../wine/dlls/xmllite/tests/reader.c:2675

This frame has 113 object(s):
    [32, 36) 'd.i439.i' (line 824)
    [48, 52) 'd.i412.i' (line 824)
    [64, 68) 'd.i385.i' (line 824)
    [80, 84) 'd.i358.i' (line 824)
...
    [2672, 2676) 'nodetype.i2' (line 620)
    [2688, 2696) 'resolver.i' (line 515)
    [2720, 2728) 'unk.i' (line 516)
    [2752, 2760) 'reader.i' (line 517)
    [2784, 2788) 'dtd.i' (line 518) <== Memory access at offset 2784 partially overflows this variable
    [2800, 2804) 'nodetype.i' (line 519)
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/dlls/xmllite/reader.c:2807:20 in xmlreader_GetProperty
Shadow bytes around the buggy address:
  0x7ffffe1ff600: f8 f2 f2 f2 f8 f2 f8 f2 f2 f2 f8 f2 f8 f2 f2 f2
  0x7ffffe1ff680: f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 f8 f2 f2 f2 f8 f2
  0x7ffffe1ff700: f8 f2 f2 f2 f8 f2 f8 f2 f2 f2 f8 f2 f8 f2 f8 f2
  0x7ffffe1ff780: f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2
  0x7ffffe1ff800: f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 00 f2 f2 f2
=>0x7ffffe1ff880: 00 f2 f2 f2 00 f2 f2 f2[04]f2 04 f3 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 00 00 00 00 00 00 00 00
  0x7ffffe1ffa00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ffffe1ffa80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ffffe1ffb00: 00 00 00 00 00 00 00 00 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
==560==ABORTING
05c0:fixme:kernelbase:AppPolicyGetProcessTerminationMethod FFFFFFFFFFFFFFFA, 00007FFFFEA8FE80
make: *** [Makefile:471569: dlls/xmllite/tests/x86_64-windows/reader.ok] Fehler 1
```

--
  v5: xmllite/tests: Avoid buffer overflow by using LONG_PTR. (ASan)

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

[PATCH v5 1/1] xmllite/tests: Avoid buffer overflow by using LONG_PTR. (ASan)

[Bernhard Übelacker]

From: Bernhard Übelacker

---
 dlls/xmllite/tests/reader.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/dlls/xmllite/tests/reader.c b/dlls/xmllite/tests/reader.c
index 6c784826232..0dd490ff989 100644
--- a/dlls/xmllite/tests/reader.c
+++ b/dlls/xmllite/tests/reader.c
@@ -515,7 +515,7 @@ static void test_reader_create(void)
     IXmlResolver *resolver;
     IUnknown *input, *unk;
     IXmlReader *reader;
-    DtdProcessing dtd;
+    LONG_PTR dtd;
     XmlNodeType nodetype;
     HRESULT hr;

@@ -578,10 +578,10 @@ static void test_reader_create(void)
     hr = IXmlReader_SetProperty(reader, XmlReaderProperty_XmlResolver, 0);
     ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);

-    dtd = 2;
-    hr = IXmlReader_GetProperty(reader, XmlReaderProperty_DtdProcessing, (LONG_PTR*)&dtd);
+    dtd = -1;
+    hr = IXmlReader_GetProperty(reader, XmlReaderProperty_DtdProcessing, &dtd);
     ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
-    ok(dtd == DtdProcessing_Prohibit, "got %d\n", dtd);
+    ok(dtd == DtdProcessing_Prohibit, "got %Id\n", dtd);

dtd = 2;
     hr = IXmlReader_SetProperty(reader, XmlReaderProperty_DtdProcessing, dtd);

[Bernhard Übelacker (＠bernhardu)]

Thanks again, yes, it is no longer needed.

v4:
- Removed no longer needed cast to LONG_PTR*.

[Nikolay Sivov (＠nsivov)]

On Thu Dec 19 19:31:44 2024 +0000, Alfred Agrell wrote:
> I'd change line 581 to something with the high half nonzero (-1, for
> example), to verify how many bytes native actually writes.
> (It writes a LONG_PTR, I checked.)

It's a natural assumption that argument type width is how many bytes it will write. You wouldn't check if e.g. 4-byte integer pointer argument updates all 4 bytes.

[PATCH v5 0/1] MR7047: xmllite/tests: Avoid buffer overflow by using LONG_PTR. (ASan) - approved

[Nikolay Sivov (＠nsivov)]

This merge request was approved by Nikolay Sivov.