[PATCH v2 0/1] MR5490: setupapi: Don't set RequiredSize when SetupDiGetClassDescription* fails.
Discovered while investigating Bug 56551. -- v2: setupapi: Don't set RequiredSize when SetupDiGetClassDescription* fails. https://gitlab.winehq.org/wine/wine/-/merge_requests/5490
From: Alex Henrie <alexhenrie24(a)gmail.com> Discovered while investigating Bug 56551. --- dlls/setupapi/devinst.c | 20 ++++++++++---------- dlls/setupapi/tests/devinst.c | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index 7e2d683daa6..43e2e071aa0 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -2119,7 +2119,7 @@ BOOL WINAPI SetupDiGetClassDescriptionExA( { HKEY hKey; DWORD dwLength; - BOOL ret; + LSTATUS ls; hKey = SetupDiOpenClassRegKeyExA(ClassGuid, KEY_ALL_ACCESS, @@ -2133,11 +2133,11 @@ BOOL WINAPI SetupDiGetClassDescriptionExA( } dwLength = ClassDescriptionSize; - ret = !RegQueryValueExA( hKey, NULL, NULL, NULL, - (LPBYTE)ClassDescription, &dwLength ); - if (RequiredSize) *RequiredSize = dwLength; + ls = RegQueryValueExA(hKey, NULL, NULL, NULL, (BYTE *)ClassDescription, &dwLength); RegCloseKey(hKey); - return ret; + if (ls != ERROR_FILE_NOT_FOUND && RequiredSize) + *RequiredSize = dwLength; + return !ls; } /*********************************************************************** @@ -2153,7 +2153,7 @@ BOOL WINAPI SetupDiGetClassDescriptionExW( { HKEY hKey; DWORD dwLength; - BOOL ret; + LSTATUS ls; hKey = SetupDiOpenClassRegKeyExW(ClassGuid, KEY_ALL_ACCESS, @@ -2167,11 +2167,11 @@ BOOL WINAPI SetupDiGetClassDescriptionExW( } dwLength = ClassDescriptionSize * sizeof(WCHAR); - ret = !RegQueryValueExW( hKey, NULL, NULL, NULL, - (LPBYTE)ClassDescription, &dwLength ); - if (RequiredSize) *RequiredSize = dwLength / sizeof(WCHAR); + ls = RegQueryValueExW(hKey, NULL, NULL, NULL, (BYTE *)ClassDescription, &dwLength); RegCloseKey(hKey); - return ret; + if (ls != ERROR_FILE_NOT_FOUND && RequiredSize) + *RequiredSize = dwLength / sizeof(WCHAR); + return !ls; } /*********************************************************************** diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c index 2dc891d61f1..8544345b669 100644 --- a/dlls/setupapi/tests/devinst.c +++ b/dlls/setupapi/tests/devinst.c @@ -162,6 +162,7 @@ static void test_install_class(void) '1','1','d','b','-','b','7','0','4','-', '0','0','1','1','9','5','5','c','2','b','d','b','}',0}; char tmpfile[MAX_PATH]; + DWORD size; BOOL ret; static const char inf_data[] = @@ -195,12 +196,27 @@ static void test_install_class(void) ok(!ret, "Expected failure.\n"); ok(GetLastError() == ERROR_FILE_NOT_FOUND, "Got unexpected error %#lx.\n", GetLastError()); + size = 123; + SetLastError(0xdeadbeef); + ret = SetupDiGetClassDescriptionA(&guid, NULL, 0, &size); + ok(!ret, "Expected failure.\n"); + ok(size == 123, "Expected 123, got %ld.\n", size); + todo_wine + ok(GetLastError() == ERROR_NOT_FOUND /* win7 */ || GetLastError() == ERROR_INVALID_CLASS /* win10 */, + "Got unexpected error %#lx.\n", GetLastError()); + /* The next call will succeed. Information is put into the registry but the * location(s) is/are depending on the Windows version. */ ret = SetupDiInstallClassA(NULL, tmpfile, 0, NULL); ok(ret, "Failed to install class, error %#lx.\n", GetLastError()); + SetLastError(0xdeadbeef); + ret = SetupDiGetClassDescriptionA(&guid, NULL, 0, &size); + todo_wine ok(!ret, "Expected failure.\n"); + ok(size == sizeof("Wine test devices"), "Expected %Iu, got %lu.\n", sizeof("Wine test devices"), size); + todo_wine ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, "Got unexpected error %#lx.\n", GetLastError()); + ret = RegDeleteKeyW(HKEY_LOCAL_MACHINE, classKey); ok(!ret, "Failed to delete class key, error %lu.\n", GetLastError()); DeleteFileA(tmpfile); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5490
On Wed Apr 24 14:29:13 2024 +0000, Elizabeth Figura wrote:
Can we check the last error, please? What if the buffer is too short? Isn't that supposed to write the required size? You're right, it depends on the error. I've pushed a new patch with a second set of tests to make that clear.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5490#note_68738
On Wed Apr 24 14:29:13 2024 +0000, Alex Henrie wrote:
You're right, it depends on the error. I've pushed a new patch with a second set of tests to make that clear. Wouldn't it make more sense to use a positive check for ERROR_INSUFFICIENT_BUFFER?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5490#note_68755
On Wed Apr 24 16:53:54 2024 +0000, Elizabeth Figura wrote:
Wouldn't it make more sense to use a positive check for ERROR_INSUFFICIENT_BUFFER? What do you mean? If RegQueryValueEx runs out of space, it returns ERROR_MORE_DATA. But if the class is installed, SetupDiGetClassDescription writes to RequiredSize whether the buffer is sufficient or not. Are you just saying that you want to see a test where SetupDiGetClassDescription succeeds and writes to both ClassDescription and RequiredSize?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5490#note_68759
What do you mean? If RegQueryValueEx runs out of space, it returns ERROR_MORE_DATA. But if the class is installed, SetupDiGetClassDescription writes to RequiredSize whether the buffer is sufficient or not.
Well, we should be translating ERROR_MORE_DATA to ERROR_INSUFFICIENT_BUFFER apparently, but what I mean is, write "ls == ERROR_INSUFFICIENT_BUFFER" instead of "ls != ERROR_FILE_NOT_FOUND".
Are you just saying that you want to see a test where SetupDiGetClassDescription succeeds and writes to both ClassDescription and RequiredSize?
That would certainly be nice to have, although I guess I wouldn't consider it necessary for the patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5490#note_68761
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Elizabeth Figura (＠zfigura)