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@vladimir.panteleev.md ---
Since v3: Apply suggestion from review and copy the innards of GetEnvironmentVariableW instead of saving and restoring the last error. --- dlls/kernel32/tests/environ.c | 8 ++++---- dlls/kernelbase/process.c | 30 +++++++++++++++++++----------- 2 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index b2c9a3a4ff..3144b8e04e 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -158,10 +158,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); @@ -276,10 +276,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 612b0b2a2f..3cd2934c20 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1232,24 +1232,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;