[PATCH 0/1] MR6203: advapi32: Avoid that invalid pointers cause crashes.

[Haoyang Chen (＠chenhaoyang)]

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

[PATCH 1/1] advapi32: Avoid that invalid pointers cause crashes.

[Haoyang Chen]

From: Haoyang Chen <chenhaoyang(a)kylinos.cn>

---
 dlls/advapi32/eventlog.c       | 21 ++++++++++++++++-----
 dlls/advapi32/tests/eventlog.c |  6 ++++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/dlls/advapi32/eventlog.c b/dlls/advapi32/eventlog.c
index 58db53f5536..4b9f059ddc8 100644
--- a/dlls/advapi32/eventlog.c
+++ b/dlls/advapi32/eventlog.c
@@ -32,6 +32,7 @@
 #include "evntprov.h"
 
 #include "wine/debug.h"
+#include "wine/exception.h"
 
 #include "advapi32_misc.h"
 
@@ -601,7 +602,7 @@ BOOL WINAPI ReportEventA ( HANDLE hEventLog, WORD wType, WORD wCategory, DWORD d
     LPWSTR *wideStrArray;
     UNICODE_STRING str;
     UINT i;
-    BOOL ret;
+    BOOL ret = FALSE;
 
     FIXME("(%p,0x%04x,0x%04x,0x%08lx,%p,0x%04x,0x%08lx,%p,%p): stub\n", hEventLog,
           wType, wCategory, dwEventID, lpUserSid, wNumStrings, dwDataSize, lpStrings, lpRawData);
@@ -610,12 +611,22 @@ BOOL WINAPI ReportEventA ( HANDLE hEventLog, WORD wType, WORD wCategory, DWORD d
     if (!lpStrings) return TRUE;
 
     wideStrArray = malloc(sizeof(WCHAR *) * wNumStrings);
-    for (i = 0; i < wNumStrings; i++)
+    __TRY
+    {
+        for (i = 0; i < wNumStrings; i++)
+        {
+            RtlCreateUnicodeStringFromAsciiz(&str, lpStrings[i]);
+            wideStrArray[i] = str.Buffer;
+        }
+        ret = TRUE;
+    }
+    __EXCEPT_PAGE_FAULT
     {
-        RtlCreateUnicodeStringFromAsciiz(&str, lpStrings[i]);
-        wideStrArray[i] = str.Buffer;
+        SetLastError(ERROR_INVALID_PARAMETER);
     }
-    ret = ReportEventW(hEventLog, wType, wCategory, dwEventID, lpUserSid,
+    __ENDTRY
+
+    if (ret) ret = ReportEventW(hEventLog, wType, wCategory, dwEventID, lpUserSid,
                        wNumStrings, dwDataSize, (LPCWSTR *)wideStrArray, lpRawData);
     for (i = 0; i < wNumStrings; i++)
         free(wideStrArray[i]);
diff --git a/dlls/advapi32/tests/eventlog.c b/dlls/advapi32/tests/eventlog.c
index 5649ca37b3b..be4d75d2b01 100644
--- a/dlls/advapi32/tests/eventlog.c
+++ b/dlls/advapi32/tests/eventlog.c
@@ -829,6 +829,12 @@ static void test_readwrite(void)
         }
     }
 
+    SetLastError(0xdeadbeef);
+    buf = (void*)0xdeadbeef;
+    ret = ReportEventA(handle, 0, 0, 0, NULL, 1, 0, buf, NULL);
+    ok(!ret, "Expected failure\n");
+    ok(GetLastError() == ERROR_INVALID_PARAMETER, "Expected ERROR_INVALID_PARAMETER, but got: %lx\n",  GetLastError());
+
     SetLastError(0xdeadbeef);
     ret = ReportEventA(handle, 0x20, 0, 0, NULL, 0, 0, NULL, NULL);
     if (!ret && GetLastError() == ERROR_CRC)
-- 
GitLab

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

[Alfred Agrell (＠Alcaro)]

Alfred Agrell (@Alcaro) commented about dlls/advapi32/tests/eventlog.c:
> }
>      }
>
> +    SetLastError(0xdeadbeef);
> +    buf = (void*)0xdeadbeef;

Can't ASLR put something there by accident, making the test flaky? Wouldn't it be better to use a guaranteed-invalid address, like 0x00000001 or a VirtualAlloc(PAGE_NOACCESS)?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6203#note_77869

[Alex Henrie (＠alexhenrie)]

Hello Haoyang, thanks for the patch! What application requires this?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6203#note_77910

[Alex Henrie (＠alexhenrie)]

On Tue Aug  6 10:46:18 2024 +0000, Alfred Agrell wrote:
> Can't ASLR put something there by accident, making the test flaky?
> Wouldn't it be better to use a guaranteed-invalid address, like
> 0x00000001 or a VirtualAlloc(PAGE_NOACCESS)?
> It's very unlikely. I wouldn't worry about it, but I'd be interested to hear what others have to say.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6203#note_77911

[Haoyang Chen (＠chenhaoyang)]

On Wed Aug  7 01:37:43 2024 +0000, Alex Henrie wrote:
> Hello Haoyang, thanks for the patch! What application requires this?
> It is AhsProtector. it is an anti-cracking engine. It calls ReportEventA during installation, but it uses a wrong string pointer. I speculate that it was coded incorrectly.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6203#note_77959