[PATCH v3 1/4] kernel32/tests: Fix GetLastError() checks in environment tests.
Lack of SetLastError() calls caused tests to compare stale errors, instead of the effects of the tested (immediately previously called) functions. Add SetLastError calls, and fix a broken test (copy check from A to W tests). Signed-off-by: Vladimir Panteleev <git(a)vladimir.panteleev.md> --- Since v2: No changes (only rebase on master). Suggested change from v2 is in next commit as it requires changes in Wine. dlls/kernel32/tests/environ.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index 44a6a0cff0..128a5fdbe5 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -131,6 +131,7 @@ static void test_GetSetEnvironmentVariableA(void) ok(ret == TRUE, "should erase existing variable\n"); lstrcpyA(buf, "foo"); + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(name, buf, lstrlenA(value) + 1); ok(lstrcmpA(buf, "foo") == 0, "should not touch the buffer\n"); ok(ret_size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND, @@ -163,16 +164,19 @@ static void test_GetSetEnvironmentVariableA(void) name, ret_size, GetLastError(), buf); /* Test the limits */ + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(NULL, NULL, 0); ok(ret_size == 0 && (GetLastError() == ERROR_INVALID_PARAMETER || GetLastError() == ERROR_ENVVAR_NOT_FOUND), "should not find variable but ret_size=%d GetLastError=%d\n", ret_size, GetLastError()); + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(NULL, buf, lstrlenA(value) + 1); ok(ret_size == 0 && (GetLastError() == ERROR_INVALID_PARAMETER || GetLastError() == ERROR_ENVVAR_NOT_FOUND), "should not find variable but ret_size=%d GetLastError=%d\n", ret_size, GetLastError()); + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA("", buf, lstrlenA(value) + 1); ok(ret_size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND, "should not find variable but ret_size=%d GetLastError=%d\n", @@ -233,6 +237,7 @@ static void test_GetSetEnvironmentVariableW(void) ok(ret == TRUE, "should erase existing variable\n"); lstrcpyW(buf, fooW); + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(name, buf, lstrlenW(value) + 1); ok(lstrcmpW(buf, fooW) == 0, "should not touch the buffer\n"); ok(ret_size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND, @@ -255,25 +260,31 @@ static void test_GetSetEnvironmentVariableW(void) ok(ret == TRUE, "should not fail with empty value but GetLastError=%d\n", GetLastError()); lstrcpyW(buf, fooW); + SetLastError(0); ret_size = GetEnvironmentVariableW(name, buf, lstrlenW(value) + 1); - ok(ret_size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND, - "should not find variable but ret_size=%d GetLastError=%d\n", + ok(ret_size == 0 && + ((GetLastError() == 0 && 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()); ok(lstrcmpW(buf, empty_strW) == 0, "should copy an empty string\n"); /* Test the limits */ + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(NULL, NULL, 0); - ok(ret_size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND, + ok(ret_size == 0 && (GetLastError() == ERROR_INVALID_PARAMETER || GetLastError() == ERROR_ENVVAR_NOT_FOUND), "should not find variable but ret_size=%d GetLastError=%d\n", ret_size, GetLastError()); if (0) /* Both tests crash on Vista */ { + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(NULL, buf, lstrlenW(value) + 1); ok(ret_size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND, "should not find variable but ret_size=%d GetLastError=%d\n", ret_size, GetLastError()); + SetLastError(0xdeadbeef); ret = SetEnvironmentVariableW(NULL, NULL); ok(ret == FALSE && (GetLastError() == ERROR_INVALID_PARAMETER || GetLastError() == ERROR_ENVVAR_NOT_FOUND), "should fail with NULL, NULL but ret=%d and GetLastError=%d\n", @@ -342,6 +353,7 @@ static void test_ExpandEnvironmentStringsA(void) ret_size = ExpandEnvironmentStringsA(buf, buf2, sizeof(buf2)); ok(!strcmp(buf, buf2), "ExpandEnvironmentStrings failed %s vs %s. ret_size = %d\n", buf, buf2, ret_size); + SetLastError(0xdeadbeef); ret_size1 = GetWindowsDirectoryA(buf1,256); ok ((ret_size1 >0) && (ret_size1<256), "GetWindowsDirectory Failed\n"); ret_size = ExpandEnvironmentStringsA("%SystemRoot%",buf,sizeof(buf)); @@ -374,6 +386,7 @@ static void test_GetComputerName(void) int name_len; size = 0; + SetLastError(0xdeadbeef); ret = GetComputerNameA((LPSTR)0xdeadbeef, &size); error = GetLastError(); ok(!ret && error == ERROR_BUFFER_OVERFLOW, "GetComputerNameA should have failed with ERROR_BUFFER_OVERFLOW instead of %d\n", error); @@ -433,6 +446,7 @@ static void test_GetComputerNameExA(void) } size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExA(ComputerNameDnsDomain, (LPSTR)0xdeadbeef, &size); error = GetLastError(); ok(ret == 0, "Expected 0, got %d\n", ret); @@ -452,6 +466,7 @@ static void test_GetComputerNameExA(void) HeapFree(GetProcessHeap(), 0, name); size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExA(ComputerNameDnsFullyQualified, (LPSTR)0xdeadbeef, &size); error = GetLastError(); ok(ret == 0, "Expected 0, got %d\n", ret); @@ -468,6 +483,7 @@ static void test_GetComputerNameExA(void) HeapFree(GetProcessHeap(), 0, name); size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExA(ComputerNameDnsHostname, (LPSTR)0xdeadbeef, &size); error = GetLastError(); ok(ret == 0, "Expected 0, got %d\n", ret); @@ -484,12 +500,14 @@ static void test_GetComputerNameExA(void) HeapFree(GetProcessHeap(), 0, name); size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExA(ComputerNameNetBIOS, (LPSTR)0xdeadbeef, &size); error = GetLastError(); ok(ret == 0, "Expected 0, got %d\n", ret); ok(error == ERROR_MORE_DATA, "Expected ERROR_MORE_DATA, got %d\n", error); size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExA(ComputerNameNetBIOS, NULL, &size); error = GetLastError(); ok(ret == 0, "Expected 0, got %d\n", ret); @@ -520,6 +538,7 @@ static void test_GetComputerNameExW(void) } size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExW(ComputerNameDnsDomain, (LPWSTR)0xdeadbeef, &size); error = GetLastError(); ok(!ret && error == ERROR_MORE_DATA, "GetComputerNameExW should have failed with ERROR_MORE_DATA instead of %d\n", error); @@ -530,6 +549,7 @@ static void test_GetComputerNameExW(void) HeapFree(GetProcessHeap(), 0, nameW); size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExW(ComputerNameDnsFullyQualified, (LPWSTR)0xdeadbeef, &size); error = GetLastError(); ok(!ret && error == ERROR_MORE_DATA, "GetComputerNameExW should have failed with ERROR_MORE_DATA instead of %d\n", error); @@ -540,6 +560,7 @@ static void test_GetComputerNameExW(void) HeapFree(GetProcessHeap(), 0, nameW); size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExW(ComputerNameDnsHostname, (LPWSTR)0xdeadbeef, &size); error = GetLastError(); ok(!ret && error == ERROR_MORE_DATA, "GetComputerNameExW should have failed with ERROR_MORE_DATA instead of %d\n", error); @@ -550,6 +571,7 @@ static void test_GetComputerNameExW(void) HeapFree(GetProcessHeap(), 0, nameW); size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExW(ComputerNameNetBIOS, (LPWSTR)0xdeadbeef, &size); error = GetLastError(); ok(!ret && error == ERROR_MORE_DATA, "GetComputerNameExW should have failed with ERROR_MORE_DATA instead of %d\n", error); @@ -560,6 +582,7 @@ static void test_GetComputerNameExW(void) HeapFree(GetProcessHeap(), 0, nameW); size = 0; + SetLastError(0xdeadbeef); ret = pGetComputerNameExW(ComputerNameNetBIOS, NULL, &size); error = GetLastError(); ok(!ret && error == ERROR_MORE_DATA, "GetComputerNameExW should have failed with ERROR_MORE_DATA instead of %d\n", error); -- 2.25.0
Do not clobber last error with NO_ERROR when GetEnvironmentVariableA succeeds, matching the behavior of GetEnvironmentVariableW and Windows. Signed-off-by: Vladimir Panteleev <git(a)vladimir.panteleev.md> --- Since v2: New patch (fix test as suggested, and fix Wine to pass the test). dlls/kernel32/tests/environ.c | 8 ++++---- dlls/kernelbase/process.c | 10 ++++++++-- 2 files changed, 12 insertions(+), 6 deletions(-) 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 a07dddb1fc..8fe5b6f495 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1234,7 +1234,9 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableA( LPCSTR name, LPSTR value { UNICODE_STRING us_name; PWSTR valueW; - DWORD ret; + DWORD ret, last_err; + + last_err = GetLastError(); /* limit the size to sane values */ size = min( size, 32767 ); @@ -1249,7 +1251,11 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableA( LPCSTR name, LPSTR value * - 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; + if (GetLastError() == 0) + { + SetLastError(last_err); + if (ret == 0 && size) value[0] = 0; + } RtlFreeUnicodeString( &us_name ); HeapFree( GetProcessHeap(), 0, valueW ); return ret; -- 2.25.0
Vladimir Panteleev <git(a)vladimir.panteleev.md> writes:
@@ -1234,7 +1234,9 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableA( LPCSTR name, LPSTR value { UNICODE_STRING us_name; PWSTR valueW; - DWORD ret; + DWORD ret, last_err; + + last_err = GetLastError();
/* limit the size to sane values */ size = min( size, 32767 ); @@ -1249,7 +1251,11 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableA( LPCSTR name, LPSTR value * - 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; + if (GetLastError() == 0) + { + SetLastError(last_err); + if (ret == 0 && size) value[0] = 0; + }
It would be cleaner to avoid changing last error at all, instead of saving and restoring it. -- Alexandre Julliard julliard(a)winehq.org
Windows doesn't do this (except XP, and then only for empty variables). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48471 Signed-off-by: Vladimir Panteleev <git(a)vladimir.panteleev.md> --- Since v2: Also test A variant, to ensure A/W behave consistently. 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 8fe5b6f495..b9686999d4 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1280,7 +1280,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.25.0
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> --- Since v2: Also test A variant, to ensure A/W behave consistently. 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 b9686999d4..606ce0caed 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1282,7 +1282,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.25.0
participants (2)
-
Alexandre Julliard -
Vladimir Panteleev