[PATCH v3 0/2] MR5519: setupapi: use INF parser to read class GUID and class name
Previously, `SetupDiGetINFClassW()` read INF files with `GetPrivateProfileString()`, which does not substitute %strkey% tokens. This caused device installation to fail for devices which had driver INF files using %strkey% tokens in Version section. An example of such device is Vernier LabQuest Mini (08f7:0008) for which Vernier's LoggerPro application includes a driver. The INF file in question adds a new device setup class and has following entries in Version section: ``` Class = %ClassName% ClassGuid = %DeviceClassGUID% ``` Strings section includes following entries: ``` DeviceClassGUID = "{6B8429BF-10AD-4b66-9FBA-2FE72B891721}" ClassName = "VST_WinUSB" ``` Previously, when LoggerPro was installed and LabQuest Mini was hotplugged, device installation failed with the following error: ``` fixme:setupapi:SetupDiGetINFClassW failed to convert "L"%DeviceClassGUID"" into a guid ``` This caused GUID_NULL to be used and Class was not set to the registry for the device. With this commit, correct class GUID and names are set to the device registry entry. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56579 -- v3: setupapi: use INF parser to read class GUID and class name setupapi/tests: add tests for reading INF class with %strkey% tokens https://gitlab.winehq.org/wine/wine/-/merge_requests/5519
From: Tuomas Räsänen <tuomas.rasanen(a)opinsys.fi> --- dlls/setupapi/tests/devinst.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c index 2dc891d61f1..6d44c2cef16 100644 --- a/dlls/setupapi/tests/devinst.c +++ b/dlls/setupapi/tests/devinst.c @@ -1876,6 +1876,8 @@ static void test_get_inf_class(void) static const char inffile[] = "winetest.inf"; static const char content[] = "[Version]\r\n\r\n"; static const char* signatures[] = {"\"$CHICAGO$\"", "\"$Windows NT$\""}; + static const GUID deadbeef_class_guid = {0xdeadbeef,0xdead,0xbeef,{0xde,0xad,0xbe,0xef,0xde,0xad,0xbe,0xef}}; + static const char deadbeef_class_name[] = "DeadBeef"; char cn[MAX_PATH]; char filename[MAX_PATH]; @@ -2014,6 +2016,24 @@ static void test_get_inf_class(void) todo_wine ok(count == 4, "expected count==4, got %lu(%s)\n", count, cn); + /* Test Strings substitution */ + WritePrivateProfileStringA("Version", "Class", "%ClassName%", filename); + WritePrivateProfileStringA("Version", "ClassGUID", "%ClassGuid%", filename); + + /* Without Strings section the ClassGUID is invalid */ + retval = SetupDiGetINFClassA(filename, &guid, cn, MAX_PATH, NULL); + ok(!retval, "expected SetupDiGetINFClassA to fail because ClassGUID is invalid (has non-substituted strkey token)!\n"); + + /* With Strings section the ClassGUID and Class should be substituted */ + WritePrivateProfileStringA("Strings", "ClassName", deadbeef_class_name, filename); + WritePrivateProfileStringA("Strings", "ClassGuid", "{deadbeef-dead-beef-dead-beefdeadbeef}", filename); + count = 0xdeadbeef; + retval = SetupDiGetINFClassA(filename, &guid, cn, MAX_PATH, &count); + ok(retval, "expected SetupDiGetINFClassA to succeed! error %lu\n", GetLastError()); + ok(count == lstrlenA(deadbeef_class_name) + 1, "expected count=%d, got %lu\n", lstrlenA(deadbeef_class_name) + 1, count); + ok(!lstrcmpA(deadbeef_class_name, cn), "expected class_name='%s', got '%s'\n", deadbeef_class_name, cn); + ok(IsEqualGUID(&deadbeef_class_guid, &guid), "expected ClassGUID to be deadbeef-dead-beef-dead-beefdeadbeef\n"); + DeleteFileA(filename); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5519
From: Tuomas Räsänen <tuomas.rasanen(a)opinsys.fi> Previously, SetupDiGetINFClassW read INF files with GetPrivateProfileString, which does not substitute %strkey% tokens. This caused device installation to fail for devices which had driver INF files using %strkey% tokens in Version section. An example of such device is Vernier LabQuest Mini (08f7:0008) for which Vernier's LoggerPro application includes a driver. The INF file in question adds a new device setup class and has following entries in Version section: Class = %ClassName% ClassGuid = %DeviceClassGUID% Strings section includes following entries: DeviceClassGUID = "{6B8429BF-10AD-4b66-9FBA-2FE72B891721}" ClassName = "VST_WinUSB" Previously, when LoggerPro was installed and LabQuest Mini was hotplugged, device installation failed with the following error: fixme:setupapi:SetupDiGetINFClassW failed to convert "L"%DeviceClassGUID"" into a guid This caused GUID_NULL to be used and Class was not set to the registry for the device. With this commit, correct class GUID and names are set to the device registry entry. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56579 --- dlls/setupapi/devinst.c | 68 ++++++++++++++++++++++++++++++++--------- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index 7e2d683daa6..95edec17137 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -4280,8 +4280,11 @@ BOOL WINAPI SetupDiGetINFClassW(PCWSTR inf, LPGUID class_guid, PWSTR class_name, DWORD size, PDWORD required_size) { BOOL have_guid, have_name; - DWORD dret; + DWORD class_name_len; WCHAR buffer[MAX_PATH]; + INFCONTEXT inf_ctx; + HINF hinf; + BOOL retval = FALSE; if (!inf) { @@ -4302,30 +4305,64 @@ BOOL WINAPI SetupDiGetINFClassW(PCWSTR inf, LPGUID class_guid, PWSTR class_name, return FALSE; } - if (!GetPrivateProfileStringW(Version, Signature, NULL, buffer, MAX_PATH, inf)) + if ((hinf = SetupOpenInfFileW(inf, NULL, INF_STYLE_WIN4, NULL)) == INVALID_HANDLE_VALUE) + { + ERR("failed to open INF file %s\n", debugstr_w(inf)); return FALSE; + } + + if (!SetupFindFirstLineW(hinf, Version, Signature, &inf_ctx)) + { + ERR("INF file %s does not have mandatory [Version].Signature\n", debugstr_w(inf)); + goto out; + } + + if (!SetupGetStringFieldW(&inf_ctx, 1, buffer, ARRAY_SIZE(buffer), NULL)) + { + ERR("failed to get [Version].Signature string from %s\n", debugstr_w(inf)); + goto out; + } if (lstrcmpiW(buffer, Chicago) && lstrcmpiW(buffer, WindowsNT)) - return FALSE; + { + ERR("INF file %s has invalid [Version].Signature: %s\n", debugstr_w(inf), debugstr_w(buffer)); + goto out; + } + + have_guid = SetupFindFirstLineW(hinf, Version, ClassGUID, &inf_ctx); - buffer[0] = '\0'; - have_guid = 0 < GetPrivateProfileStringW(Version, ClassGUID, NULL, buffer, MAX_PATH, inf); if (have_guid) { + if (!SetupGetStringFieldW(&inf_ctx, 1, buffer, ARRAY_SIZE(buffer), NULL)) + { + ERR("failed to get [Version].ClassGUID as a string from '%s'\n", debugstr_w(inf)); + goto out; + } + buffer[lstrlenW(buffer)-1] = 0; if (RPC_S_OK != UuidFromStringW(buffer + 1, class_guid)) { - FIXME("failed to convert \"%s\" into a guid\n", debugstr_w(buffer)); + ERR("INF file %s has invalid [Version].ClassGUID: %s\n", debugstr_w(inf), debugstr_w(buffer)); SetLastError(ERROR_INVALID_PARAMETER); - return FALSE; + goto out; } } - buffer[0] = '\0'; - dret = GetPrivateProfileStringW(Version, Class, NULL, buffer, MAX_PATH, inf); - have_name = 0 < dret; + have_name = SetupFindFirstLineW(hinf, Version, Class, &inf_ctx); - if (dret >= MAX_PATH -1) FIXME("buffer might be too small\n"); + class_name_len = 0; + if (have_name) + { + if (!SetupGetStringFieldW(&inf_ctx, 1, buffer, ARRAY_SIZE(buffer), NULL)) + { + ERR("failed to get [Version].Class as a string from '%s'\n", debugstr_w(inf)); + goto out; + } + + class_name_len = lstrlenW(buffer); + } + + if (class_name_len >= MAX_PATH - 1) FIXME("buffer might be too small\n"); if (have_guid && !have_name) { class_name[0] = '\0'; @@ -4334,7 +4371,7 @@ BOOL WINAPI SetupDiGetINFClassW(PCWSTR inf, LPGUID class_guid, PWSTR class_name, if (have_name) { - if (dret < size) lstrcpyW(class_name, buffer); + if (class_name_len < size) lstrcpyW(class_name, buffer); else { SetLastError(ERROR_INSUFFICIENT_BUFFER); @@ -4342,9 +4379,12 @@ BOOL WINAPI SetupDiGetINFClassW(PCWSTR inf, LPGUID class_guid, PWSTR class_name, } } - if (required_size) *required_size = dret + ((dret) ? 1 : 0); + if (required_size) *required_size = class_name_len + ((class_name_len) ? 1 : 0); - return (have_guid || have_name); + retval = (have_guid || have_name); +out: + SetupCloseInfFile(hinf); + return retval; } static LSTATUS get_device_property(struct device *device, const DEVPROPKEY *prop_key, DEVPROPTYPE *prop_type, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5519
On Sat Apr 20 12:47:13 2024 +0000, Elizabeth Figura wrote:
Ouch, I can't believe we were using profile APIs for this. Any chance we can get a test for this as well? Sure! There was an existing test case for reading INF class, so I just added couple of tests to the end. Interestingly, the test code used profile API to write to the INF file, so I just decided to go with the flow and used `WritePrivateProfileStringA()` too.
So, now there are tests, which use profile API to write to an INF file, to ensure the code it is testing does **not** use profile API to read from an INF file. How ironic! But I think that's fine, given that all we need from testing point of view is the ability to write variable definitions to sections. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68269
@zfigura My end goal is to make [Vernier's LabQuest Mini](https://www.vernier.com/product/original-labquest-mini/) work with Wine. I don't know how much work it requires, or if it's even possible, in a practical sense. I'm not (yet) experienced with Wine codebase (nor with Win API), but by looking at setupapi, pnp, etc., I get a feeling that it **should** be possible. My current understanding, based on log messages, is that the main reason why it's not already working is that `load_function_driver()` cannot find the driver (the driver does not get installed correctly) on the device startup. And that there are few obstacles before that, the bug this MR tries to solve being the first one. So, what do you think? Is there hope and if yes, what would be the most obvious things I should be looking at next? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68275
On Sat Apr 20 12:47:13 2024 +0000, Tuomas Räsänen wrote:
Sure! There was an existing test case for reading INF class, so I just added couple of tests to the end. Interestingly, the test code used profile API to write to the INF file, so I just decided to go with the flow and used `WritePrivateProfileStringA()` too. So, now there are tests, which use profile API to write to an INF file, to ensure the code it is testing does **not** use profile API to read from an INF file. How ironic! But I think that's fine, given that all we need from testing point of view is the ability to write variable definitions to sections. Yeah, it's not ideal that the tests are using profile APIs either, but I don't think we really care about that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68512
The tests are missing any todo_wine annotations, which is suspect. We need the tests to pass after every commit, which entails adding and removing todo_wine accordingly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68513
Elizabeth Figura (@zfigura) commented about dlls/setupapi/tests/devinst.c:
todo_wine ok(count == 4, "expected count==4, got %lu(%s)\n", count, cn);
+ /* Test Strings substitution */ + WritePrivateProfileStringA("Version", "Class", "%ClassName%", filename); + WritePrivateProfileStringA("Version", "ClassGUID", "%ClassGuid%", filename); + + /* Without Strings section the ClassGUID is invalid */ + retval = SetupDiGetINFClassA(filename, &guid, cn, MAX_PATH, NULL); + ok(!retval, "expected SetupDiGetINFClassA to fail because ClassGUID is invalid (has non-substituted strkey token)!\n");
This should test the last error. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68514
Elizabeth Figura (@zfigura) commented about dlls/setupapi/devinst.c:
+ have_name = SetupFindFirstLineW(hinf, Version, Class, &inf_ctx);
- if (dret >= MAX_PATH -1) FIXME("buffer might be too small\n"); + class_name_len = 0; + if (have_name) + { + if (!SetupGetStringFieldW(&inf_ctx, 1, buffer, ARRAY_SIZE(buffer), NULL)) + { + ERR("failed to get [Version].Class as a string from '%s'\n", debugstr_w(inf)); + goto out; + } + + class_name_len = lstrlenW(buffer); + } + + if (class_name_len >= MAX_PATH - 1) FIXME("buffer might be too small\n"); That's impossible; "buffer" is of length MAX_PATH. If we're going to overrun the buffer then SetupGetStringFieldW() would have failed anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68515
On Wed Apr 24 19:10:40 2024 +0000, Tuomas Räsänen wrote:
@zfigura My end goal is to make [Vernier's LabQuest Mini](https://www.vernier.com/product/original-labquest-mini/) work with Wine. I don't know how much work it requires, or if it's even possible, in a practical sense. I'm not (yet) experienced with Wine codebase (nor with Win API), but by looking at setupapi, pnp, etc., I get a feeling that it **should** be possible. My current understanding, based on log messages, is that the main reason why it's not already working is that `load_function_driver()` cannot find the driver (the driver does not get installed correctly) on the device startup. And that there are few obstacles before that, the bug this MR tries to solve being the first one. So, what do you think? Is there hope and if yes, what would be the most obvious things I should be looking at next? Hard to say without knowing all the details, but a piece of domain-specific hardware which communicates via USB is generally plausible.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68516
On Tue Apr 23 00:28:58 2024 +0000, Elizabeth Figura wrote:
That's impossible; "buffer" is of length MAX_PATH. If we're going to overrun the buffer then SetupGetStringFieldW() would have failed anyway. This conditional FIXME was there already, so no change here (other than the local variable rename).
But now that I just wrote that, I think I got what you mean and **what this conditional FIXME was used for previously**. So this FIXME was never about detecting overruns, because the overrun was impossible also previously. But it's point is to issue a warning that `MAX_PATH` might not be enough to capture the whole value of the key, i.e. if there was a risk to get a truncated value; if the buffer gets completely filled, it's possible that part of the value got left out. But now, `SetupGetStringFieldW` parses the whole value and checks if the buffer is actually big enough to store it, so indeed, this FIXME is completely redundant. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68762
participants (2)
-
Elizabeth Figura (＠zfigura) -
Tuomas Räsänen