[PATCH v2 1/4] advapi32: Set last error directly in GetUserName.
Signed-off-by: Gijs Vermeulen <gijsvrm(a)gmail.com> --- dlls/advapi32/advapi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/advapi32/advapi.c b/dlls/advapi32/advapi.c index cfd79e02af..735b1db7c3 100644 --- a/dlls/advapi32/advapi.c +++ b/dlls/advapi32/advapi.c @@ -48,6 +48,7 @@ BOOL WINAPI GetUserNameA( LPSTR name, LPDWORD size ) BOOL ret; if (!len) return FALSE; + if (len > *size) SetLastError(ERROR_INSUFFICIENT_BUFFER); if ((ret = (len < *size))) len++; *size = len; return ret; @@ -63,6 +64,7 @@ BOOL WINAPI GetUserNameW( LPWSTR name, LPDWORD size ) BOOL ret; if (!len) return FALSE; + if (len > *size) SetLastError(ERROR_INSUFFICIENT_BUFFER); if ((ret = (len < *size))) len++; *size = len; return ret; -- 2.26.2
From: Vladimir Panteleev <git(a)vladimir.panteleev.md> Avoid clobbering last error with NO_ERROR when GetEnvironmentVariableA succeeds, matching the behavior of GetEnvironmentVariableW and Windows. Instead of naively saving and restoring the last error, call RtlQueryEnvironmentVariable_U directly to avoid unnecessarily setting it in the first place. Signed-off-by: Vladimir Panteleev <git(a)vladimir.panteleev.md> Signed-off-by: Gijs Vermeulen <gijsvrm(a)gmail.com> --- dlls/advapi32/tests/security.c | 2 ++ dlls/kernel32/tests/environ.c | 8 ++++---- dlls/kernelbase/process.c | 30 +++++++++++++++++++----------- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index c6f5d4690a..825f845190 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -5099,6 +5099,7 @@ static void test_GetUserNameA(void) ok(buffer_len == required_len || broken(buffer_len == required_len / sizeof(WCHAR)), /* XP+ */ "Outputted buffer length was %u\n", buffer_len); + ok(GetLastError() == 0xdeadbeef, "Last error was %u\n", GetLastError()); /* Use the reported buffer size from the last GetUserNameA call and pass * a length that is one less than the required value. */ @@ -5168,6 +5169,7 @@ static void test_GetUserNameW(void) ok(ret == TRUE, "GetUserNameW returned %d, last error %u\n", ret, GetLastError()); ok(memcmp(buffer, filler, sizeof(filler)) != 0, "Output buffer was untouched\n"); ok(buffer_len == required_len, "Outputted buffer length was %u\n", buffer_len); + ok(GetLastError() == 0xdeadbeef, "Last error was %u\n", GetLastError()); /* GetUserNameW on XP and newer writes a truncated portion of the username string to the buffer. */ SetLastError(0xdeadbeef); diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index 128a5fdbe5..561fc71c4c 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -155,10 +155,10 @@ static void test_GetSetEnvironmentVariableA(void) "should not fail with empty value but GetLastError=%d\n", GetLastError()); lstrcpyA(buf, "foo"); - SetLastError(0); + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(name, buf, lstrlenA(value) + 1); ok(ret_size == 0 && - ((GetLastError() == 0 && lstrcmpA(buf, "") == 0) || + ((GetLastError() == 0xdeadbeef && lstrcmpA(buf, "") == 0) || (GetLastError() == ERROR_ENVVAR_NOT_FOUND)), "%s should be set to \"\" (NT) or removed (Win9x) but ret_size=%d GetLastError=%d and buf=%s\n", name, ret_size, GetLastError(), buf); @@ -260,10 +260,10 @@ static void test_GetSetEnvironmentVariableW(void) ok(ret == TRUE, "should not fail with empty value but GetLastError=%d\n", GetLastError()); lstrcpyW(buf, fooW); - SetLastError(0); + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(name, buf, lstrlenW(value) + 1); ok(ret_size == 0 && - ((GetLastError() == 0 && lstrcmpW(buf, empty_strW) == 0) || + ((GetLastError() == 0xdeadbeef && lstrcmpW(buf, empty_strW) == 0) || (GetLastError() == ERROR_ENVVAR_NOT_FOUND)), "should be set to \"\" (NT) or removed (Win9x) but ret_size=%d GetLastError=%d\n", ret_size, GetLastError()); diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 3369180206..4df5408ea9 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1234,24 +1234,32 @@ LPWSTR WINAPI DECLSPEC_HOTPATCH GetEnvironmentStringsW(void) */ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableA( LPCSTR name, LPSTR value, DWORD size ) { - UNICODE_STRING us_name; + UNICODE_STRING us_name, us_value; PWSTR valueW; - DWORD ret; + NTSTATUS status; + DWORD len, ret; /* limit the size to sane values */ size = min( size, 32767 ); if (!(valueW = HeapAlloc( GetProcessHeap(), 0, size * sizeof(WCHAR) ))) return 0; RtlCreateUnicodeStringFromAsciiz( &us_name, name ); - SetLastError( 0 ); - ret = GetEnvironmentVariableW( us_name.Buffer, valueW, size); - if (ret && ret < size) WideCharToMultiByte( CP_ACP, 0, valueW, ret + 1, value, size, NULL, NULL ); - - /* this is needed to tell, with 0 as a return value, the difference between: - * - an error (GetLastError() != 0) - * - returning an empty string (in this case, we need to update the buffer) - */ - if (ret == 0 && size && GetLastError() == 0) value[0] = 0; + us_value.Length = 0; + us_value.MaximumLength = (size ? size - 1 : 0) * sizeof(WCHAR); + us_value.Buffer = valueW; + + status = RtlQueryEnvironmentVariable_U( NULL, &us_name, &us_value ); + len = us_value.Length / sizeof(WCHAR); + if (status == STATUS_BUFFER_TOO_SMALL) ret = len + 1; + else if (!set_ntstatus( status )) ret = 0; + else if (!size) ret = len + 1; + else + { + if (len) WideCharToMultiByte( CP_ACP, 0, valueW, len + 1, value, size, NULL, NULL ); + value[len] = 0; + ret = len; + } + RtlFreeUnicodeString( &us_name ); HeapFree( GetProcessHeap(), 0, valueW ); return ret; -- 2.26.2
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=71768 Your paranoid android. === debiant (32 bit Japanese:Japan report) === advapi32: security.c:5102: Test failed: Last error was 122 === debiant (32 bit Chinese:China report) === advapi32: security.c:5102: Test failed: Last error was 122
Op do 14 mei 2020 om 23:30 schreef Marvin <testbot(a)winehq.org>:
=== debiant (32 bit Japanese:Japan report) ===
advapi32: security.c:5102: Test failed: Last error was 122
=== debiant (32 bit Chinese:China report) ===
advapi32: security.c:5102: Test failed: Last error was 122
I can't reproduce this locally. GetUserNameA() still reports success (so len < *size, that means ERROR_INSUFFICIENT_BUFFER isn't set there) and GetEnvironmentVariableA doesn't set ERROR_INSUFFICIENT_BUFFER anymore. Am I missing something here?
From: Vladimir Panteleev <git(a)vladimir.panteleev.md> Windows doesn't do this (except XP, and then only for empty variables, where it returns ERROR_MORE_DATA). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48471 Signed-off-by: Vladimir Panteleev <git(a)vladimir.panteleev.md> Signed-off-by: Gijs Vermeulen <gijsvrm(a)gmail.com> --- dlls/kernel32/tests/environ.c | 6 ++++++ dlls/kernelbase/process.c | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index 561fc71c4c..ff822d0349 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -104,9 +104,12 @@ static void test_GetSetEnvironmentVariableA(void) GetLastError()); /* Try to retrieve the environment variable we just set */ + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(name, NULL, 0); ok(ret_size == strlen(value) + 1, "should return length with terminating 0 ret_size=%d\n", ret_size); + ok(GetLastError() == 0xdeadbeef, + "should not fail with zero size but GetLastError=%d\n", GetLastError()); lstrcpyA(buf, "foo"); ret_size = GetEnvironmentVariableA(name, buf, lstrlenA(value)); @@ -206,10 +209,13 @@ static void test_GetSetEnvironmentVariableW(void) GetLastError()); /* Try to retrieve the environment variable we just set */ + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(name, NULL, 0); ok(ret_size == lstrlenW(value) + 1, "should return length with terminating 0 ret_size=%d\n", ret_size); + ok(GetLastError() == 0xdeadbeef, + "should not fail with zero size but GetLastError=%d\n", GetLastError()); lstrcpyW(buf, fooW); ret_size = GetEnvironmentVariableW(name, buf, lstrlenW(value)); diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 4df5408ea9..a82597032d 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1284,7 +1284,8 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableW( LPCWSTR name, LPWSTR val status = RtlQueryEnvironmentVariable_U( NULL, &us_name, &us_value ); len = us_value.Length / sizeof(WCHAR); - if (!set_ntstatus( status )) return (status == STATUS_BUFFER_TOO_SMALL) ? len + 1 : 0; + if (status == STATUS_BUFFER_TOO_SMALL) return len + 1; + if (!set_ntstatus( status )) return 0; if (size) val[len] = 0; return len; } -- 2.26.2
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=71769 Your paranoid android. === debiant (32 bit Chinese:China report) === advapi32: security.c:5102: Test failed: Last error was 122
From: Vladimir Panteleev <git(a)vladimir.panteleev.md> Do the same thing as Vista and up. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48471 Signed-off-by: Vladimir Panteleev <git(a)vladimir.panteleev.md> Signed-off-by: Gijs Vermeulen <gijsvrm(a)gmail.com> --- dlls/kernel32/tests/environ.c | 20 ++++++++++++++++++++ dlls/kernelbase/process.c | 3 ++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index ff822d0349..3144b8e04e 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -166,6 +166,16 @@ static void test_GetSetEnvironmentVariableA(void) "%s should be set to \"\" (NT) or removed (Win9x) but ret_size=%d GetLastError=%d and buf=%s\n", name, ret_size, GetLastError(), buf); + SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableA(name, NULL, 0); + ok(ret_size == 1 || + broken(ret_size == 0), /* XP */ + "should return 1 for empty string but ret_size=%d GetLastError=%d\n", + ret_size, GetLastError()); + ok(GetLastError() == 0xdeadbeef || + broken(GetLastError() == ERROR_MORE_DATA), /* XP */ + "should not fail with zero size but GetLastError=%d\n", GetLastError()); + /* Test the limits */ SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(NULL, NULL, 0); @@ -275,6 +285,16 @@ static void test_GetSetEnvironmentVariableW(void) ret_size, GetLastError()); ok(lstrcmpW(buf, empty_strW) == 0, "should copy an empty string\n"); + SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableW(name, NULL, 0); + ok(ret_size == 1 || + broken(ret_size == 0), /* XP */ + "should return 1 for empty string but ret_size=%d GetLastError=%d\n", + ret_size, GetLastError()); + ok(GetLastError() == 0xdeadbeef || + broken(GetLastError() == ERROR_MORE_DATA), /* XP */ + "should not fail with zero size but GetLastError=%d\n", GetLastError()); + /* Test the limits */ SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(NULL, NULL, 0); diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index a82597032d..75a97e9e7c 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1286,7 +1286,8 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableW( LPCWSTR name, LPWSTR val len = us_value.Length / sizeof(WCHAR); if (status == STATUS_BUFFER_TOO_SMALL) return len + 1; if (!set_ntstatus( status )) return 0; - if (size) val[len] = 0; + if (!size) return len + 1; + val[len] = 0; return len; } -- 2.26.2
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=71770 Your paranoid android. === debiant (32 bit Chinese:China report) === advapi32: security.c:5102: Test failed: Last error was 122
participants (2)
-
Gijs Vermeulen -
Marvin