[PATCH v2 0/1] MR5490: setupapi: Don't set RequiredSize when SetupDiGetClassDescription* fails.

[Alex Henrie (＠alexhenrie)]

Discovered while investigating Bug 56551.

--
  v2: setupapi: Don't set RequiredSize when SetupDiGetClassDescription* fails.

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

[PATCH v2 1/1] setupapi: Don't set RequiredSize when SetupDiGetClassDescription* fails.

[Alex Henrie]

From: Alex Henrie

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);

[Alex Henrie (＠alexhenrie)]

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.

[Elizabeth Figura (＠zfigura)]

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?

[Alex Henrie (＠alexhenrie)]

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?

[Elizabeth Figura (＠zfigura)]

> 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.