[PATCH v4 0/1] MR8125: mscoree: Add missing NUL terminator.

[Yuxuan Shui (＠yshui)]

Fix out-of-bound access by wcslen. Found by ASan.

--
  v4: mscoree: Add missing NUL terminator.

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

[PATCH v4 1/1] mscoree: Add missing NUL terminator.

[Yuxuan Shui]

From: Yuxuan Shui

Fix out-of-bound access by wcslen. Found by ASan.
---
 dlls/mscoree/metahost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/mscoree/metahost.c b/dlls/mscoree/metahost.c
index e1dd00656e9..1628436ac6f 100644
--- a/dlls/mscoree/metahost.c
+++ b/dlls/mscoree/metahost.c
@@ -842,7 +842,7 @@ static BOOL get_mono_path_datadir(LPWSTR path)
 {
     static const WCHAR winedatadirW[] = {'W','I','N','E','D','A','T','A','D','I','R',0};
     static const WCHAR winebuilddirW[] = {'W','I','N','E','B','U','I','L','D','D','I','R',0};
-    static const WCHAR unix_prefix[] = {'\','?','?','\','u','n','i','x','\'};
+    static const WCHAR unix_prefix[] = {'\','?','?','\','u','n','i','x','\',0};
     static const WCHAR monoW[] = {'\','m','o','n','o',0};
     static const WCHAR dotdotmonoW[] = {'\','.','.','\','m','o','n','o',0};
     const WCHAR *data_dir, *suffix;

[Yuxuan Shui (＠yshui)]

oh wait, this one is a `sizeof`, not strlen, so this one is fine. sorry about that.

[Esme Povirk (＠madewokherd)]

This should probably be a memcmp/sizeof like the other one.

[Yuxuan Shui (＠yshui)]

On Fri May 23 10:06:05 2025 +0000, Esme Povirk wrote:
> This should probably be a memcmp/sizeof like the other one.

wouldn't memcmp access out-of-bound if `data_dir` is not long enough? the same goes for the other one actually.

[Esme Povirk (＠madewokherd)]

On Fri May 23 10:06:05 2025 +0000, Yuxuan Shui wrote:
> wouldn't memcmp access out-of-bound if `data_dir` is not long enough?
> the same goes for the other one actually.

`data_dir` should be null-terminated, so if it's not long enough then it should mismatch at the null terminator and memcmp should stop there.

[Elizabeth Figura (＠zfigura)]

> `data_dir` should be null-terminated, so if it's not long enough then it should mismatch at the null terminator and memcmp should stop there.

I don't think that's how memcmp() works? The standard is a bit vague, but I'm inclined to read that it's allowed to read all <size> bytes of both memory areas. Certainly standard libraries tend to take advantage of that by doing comparisons of larger words at a time. I guess it's impossible for that to actually break, but it still seems like it should be more correct, even idiomatic, to actually check "strlen(dir) > sizeof(unix_prefix)" first.