[PATCH v3] gdi32: Create the registry keys recursively, if needed.
Fixes a regression introduced by 1cdc74b2d62d1c94005c46f9c8f4b566aa8bdcbd when creating new prefixes, as NtCreateKey does not recursively create keys. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Most of the code is taken from kernelbase's create_key, but simplified to gdi32's cases only. v3: Minor whitespace issue... dlls/gdi32/font.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/dlls/gdi32/font.c b/dlls/gdi32/font.c index 98c70a6..87d7934 100644 --- a/dlls/gdi32/font.c +++ b/dlls/gdi32/font.c @@ -29,6 +29,8 @@ #include <stdlib.h> #include <string.h> #include <assert.h> +#include "ntstatus.h" +#define WIN32_NO_STATUS #include "winerror.h" #include "windef.h" #include "winbase.h" @@ -569,11 +571,13 @@ static HKEY reg_open_key( HKEY root, const WCHAR *name, ULONG name_len ) return ret; } +/* wrapper for NtCreateKey that creates the key recursively if necessary */ static HKEY reg_create_key( HKEY root, const WCHAR *name, ULONG name_len, DWORD options, DWORD *disposition ) { UNICODE_STRING nameW = { name_len, name_len, (WCHAR *)name }; OBJECT_ATTRIBUTES attr; + NTSTATUS status; HANDLE ret; attr.Length = sizeof(attr); @@ -583,7 +587,28 @@ static HKEY reg_create_key( HKEY root, const WCHAR *name, ULONG name_len, attr.SecurityDescriptor = NULL; attr.SecurityQualityOfService = NULL; - if (NtCreateKey( &ret, MAXIMUM_ALLOWED, &attr, 0, NULL, options, disposition )) return 0; + status = NtCreateKey( &ret, MAXIMUM_ALLOWED, &attr, 0, NULL, options, disposition ); + if (status == STATUS_OBJECT_NAME_NOT_FOUND) + { + DWORD pos = 0, i = 0, len = name_len / sizeof(WCHAR); + + while (i < len && name[i] != '\\') i++; + if (i == len) return 0; + for (;;) + { + nameW.Buffer = (WCHAR *)name + pos; + nameW.Length = (i - pos) * sizeof(WCHAR); + status = NtCreateKey( &ret, MAXIMUM_ALLOWED, &attr, 0, NULL, options, disposition ); + + if (attr.RootDirectory != root) NtClose( attr.RootDirectory ); + if (!NT_SUCCESS(status)) return 0; + if (i == len) break; + attr.RootDirectory = ret; + while (i < len && name[i] == '\\') i++; + pos = i; + while (i < len && name[i] != '\\') i++; + } + } return ret; } -- 2.31.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99265 Your paranoid android. === debiant2 (32 bit report) === gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150 === debiant2 (32 bit Chinese:China report) === gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L"8" font.c:5060: Test failed: Unexpected first glyph L"8" font.c:7484: Test failed: expected 0, got -11 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 178 metafile.c:5180: Test failed: expected 0, got 178 === debiant2 (32 bit WoW report) === gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150 === debiant2 (64 bit WoW report) === gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150
On Sun, Oct 03, 2021 at 08:33:41AM -0500, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99265
Your paranoid android.
=== debiant2 (32 bit report) ===
gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150
These do indeed appear to be new failures. Huw.
On 04/10/2021 09:39, Huw Davies wrote:
On Sun, Oct 03, 2021 at 08:33:41AM -0500, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99265
Your paranoid android.
=== debiant2 (32 bit report) ===
gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150
These do indeed appear to be new failures.
Huw.
Well, I get those tests even without this patch applied. I don't actually see how it *could* influence the tests anyway, because it first tries to open the key directly (like the helper in kernelbase) so as long as the key exists, the behavior should be identical to before. Maybe the testbot created new prefixes and the tests were simply broken before and not tested somehow? At least when creating a 32-bit prefix, my window decorator crashed without this patch. Thanks, Gabriel
On 04/10/2021 16:16, Gabriel Ivăncescu wrote:
On 04/10/2021 09:39, Huw Davies wrote:
On Sun, Oct 03, 2021 at 08:33:41AM -0500, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99265
Your paranoid android.
=== debiant2 (32 bit report) ===
gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150
These do indeed appear to be new failures.
Huw.
Well, I get those tests even without this patch applied. I don't actually see how it *could* influence the tests anyway, because it first tries to open the key directly (like the helper in kernelbase) so as long as the key exists, the behavior should be identical to before.
Maybe the testbot created new prefixes and the tests were simply broken before and not tested somehow? At least when creating a 32-bit prefix, my window decorator crashed without this patch.
Thanks, Gabriel
So it looks like 8e40e56b79c39356a981bbc25100daa9635b29f3 is the culprit of the test failures, but note that the failures happen only when *creating* the prefix with that commit, and it's what's broken. If the prefix was created without that commit, it will succeed. Also, if the prefix was created with that commit, and then wine without such commit was used to run the tests, it would also fail. Anyway, it's unrelated to this patch (I even tested with it to create prefixes cleanly without crashing).
On 04/10/2021 18:41, Gabriel Ivăncescu wrote:
On 04/10/2021 16:16, Gabriel Ivăncescu wrote:
On 04/10/2021 09:39, Huw Davies wrote:
On Sun, Oct 03, 2021 at 08:33:41AM -0500, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99265
Your paranoid android.
=== debiant2 (32 bit report) ===
gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150
These do indeed appear to be new failures.
Huw.
Well, I get those tests even without this patch applied. I don't actually see how it *could* influence the tests anyway, because it first tries to open the key directly (like the helper in kernelbase) so as long as the key exists, the behavior should be identical to before.
Maybe the testbot created new prefixes and the tests were simply broken before and not tested somehow? At least when creating a 32-bit prefix, my window decorator crashed without this patch.
Thanks, Gabriel
So it looks like 8e40e56b79c39356a981bbc25100daa9635b29f3 is the culprit of the test failures, but note that the failures happen only when *creating* the prefix with that commit, and it's what's broken.
If the prefix was created without that commit, it will succeed. Also, if the prefix was created with that commit, and then wine without such commit was used to run the tests, it would also fail.
Anyway, it's unrelated to this patch (I even tested with it to create prefixes cleanly without crashing).
The problem is that these values are not set anymore in the registry: [System\CurrentControlSet\Hardware Profiles\Current\Software\Fonts] "FIXEDFON.FON"="vgafix.fon" "FONTS.FON"="vgasys.fon" "OEMFONT.FON"="vgaoem.fon" This is because, in kernelbase, we have get_special_root_hkey and create_special_root_hkey, which creates the HKEY_CURRENT_CONFIG special key. This has to be created *before* the values are set in update_codepage! However, commit 8e40e56b79c39356a981bbc25100daa9635b29f3 removed the get_dpi function, which did a get_reg_dword(HKEY_CURRENT_CONFIG, ...), which implicitly created the special key HKEY_CURRENT_CONFIG in kernelbase. I'm not sure why it works if the prefix creation font init fails. You can just forcefully hack it to fail and it will work to create those values, although it crashes the decorator and all that. So, making the wine_fonts_key creation fail for some reason will enable those values to be set. But that's obviously a hack, so the patch isn't wrong (because it makes it work). I have no idea how to fix this, since that whole special key thing is part of kernelbase. Any thoughts?
On 10/4/21 7:29 PM, Gabriel Ivăncescu wrote:
On 04/10/2021 18:41, Gabriel Ivăncescu wrote:
On 04/10/2021 16:16, Gabriel Ivăncescu wrote:
On 04/10/2021 09:39, Huw Davies wrote:
On Sun, Oct 03, 2021 at 08:33:41AM -0500, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99265
Your paranoid android.
=== debiant2 (32 bit report) ===
gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150
These do indeed appear to be new failures.
Huw.
Well, I get those tests even without this patch applied. I don't actually see how it *could* influence the tests anyway, because it first tries to open the key directly (like the helper in kernelbase) so as long as the key exists, the behavior should be identical to before.
Maybe the testbot created new prefixes and the tests were simply broken before and not tested somehow? At least when creating a 32-bit prefix, my window decorator crashed without this patch.
Thanks, Gabriel
So it looks like 8e40e56b79c39356a981bbc25100daa9635b29f3 is the culprit of the test failures, but note that the failures happen only when *creating* the prefix with that commit, and it's what's broken.
If the prefix was created without that commit, it will succeed. Also, if the prefix was created with that commit, and then wine without such commit was used to run the tests, it would also fail.
Anyway, it's unrelated to this patch (I even tested with it to create prefixes cleanly without crashing).
The problem is that these values are not set anymore in the registry:
[System\CurrentControlSet\Hardware Profiles\Current\Software\Fonts] "FIXEDFON.FON"="vgafix.fon" "FONTS.FON"="vgasys.fon" "OEMFONT.FON"="vgaoem.fon"
This is because, in kernelbase, we have get_special_root_hkey and create_special_root_hkey, which creates the HKEY_CURRENT_CONFIG special key. This has to be created *before* the values are set in update_codepage!
However, commit 8e40e56b79c39356a981bbc25100daa9635b29f3 removed the get_dpi function, which did a get_reg_dword(HKEY_CURRENT_CONFIG, ...), which implicitly created the special key HKEY_CURRENT_CONFIG in kernelbase.
I'm not sure why it works if the prefix creation font init fails. You can just forcefully hack it to fail and it will work to create those values, although it crashes the decorator and all that. So, making the wine_fonts_key creation fail for some reason will enable those values to be set. But that's obviously a hack, so the patch isn't wrong (because it makes it work).
I have no idea how to fix this, since that whole special key thing is part of kernelbase. Any thoughts?
Thanks for looking at this. The problem is that reg_create_key doesn't handle well creation of root-less keys. It shouldn't try to create "\Registry" key. The attached change helps here, does it work for you? Thanks, Jacek
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99338 Your paranoid android. === debiant2 (build log) === error: patch failed: dlls/gdi32/font.c:587 Task: Patch failed to apply === debiant2 (build log) === error: patch failed: dlls/gdi32/font.c:587 Task: Patch failed to apply
On 04/10/2021 21:03, Jacek Caban wrote:
On 10/4/21 7:29 PM, Gabriel Ivăncescu wrote:
On 04/10/2021 18:41, Gabriel Ivăncescu wrote:
On 04/10/2021 16:16, Gabriel Ivăncescu wrote:
On 04/10/2021 09:39, Huw Davies wrote:
On Sun, Oct 03, 2021 at 08:33:41AM -0500, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99265
Your paranoid android.
=== debiant2 (32 bit report) ===
gdi32: font.c:1234: Test failed: GetCharABCWidthsW should have failed font.c:1681: Test failed: GetGlyphIndicesW should have returned a 001f not 0000 font.c:4387: Test failed: info[0] = 3 for the system font font.c:5033: Test failed: Unexpected first glyph L":" font.c:5060: Test failed: Unexpected first glyph L":" font.c:7484: Test failed: expected 0, got -1 font.c:7485: Test failed: expected 0, got -6 metafile.c:5106: Test failed: expected 0, got 150 metafile.c:5180: Test failed: expected 0, got 150
These do indeed appear to be new failures.
Huw.
Well, I get those tests even without this patch applied. I don't actually see how it *could* influence the tests anyway, because it first tries to open the key directly (like the helper in kernelbase) so as long as the key exists, the behavior should be identical to before.
Maybe the testbot created new prefixes and the tests were simply broken before and not tested somehow? At least when creating a 32-bit prefix, my window decorator crashed without this patch.
Thanks, Gabriel
So it looks like 8e40e56b79c39356a981bbc25100daa9635b29f3 is the culprit of the test failures, but note that the failures happen only when *creating* the prefix with that commit, and it's what's broken.
If the prefix was created without that commit, it will succeed. Also, if the prefix was created with that commit, and then wine without such commit was used to run the tests, it would also fail.
Anyway, it's unrelated to this patch (I even tested with it to create prefixes cleanly without crashing).
The problem is that these values are not set anymore in the registry:
[System\CurrentControlSet\Hardware Profiles\Current\Software\Fonts] "FIXEDFON.FON"="vgafix.fon" "FONTS.FON"="vgasys.fon" "OEMFONT.FON"="vgaoem.fon"
This is because, in kernelbase, we have get_special_root_hkey and create_special_root_hkey, which creates the HKEY_CURRENT_CONFIG special key. This has to be created *before* the values are set in update_codepage!
However, commit 8e40e56b79c39356a981bbc25100daa9635b29f3 removed the get_dpi function, which did a get_reg_dword(HKEY_CURRENT_CONFIG, ...), which implicitly created the special key HKEY_CURRENT_CONFIG in kernelbase.
I'm not sure why it works if the prefix creation font init fails. You can just forcefully hack it to fail and it will work to create those values, although it crashes the decorator and all that. So, making the wine_fonts_key creation fail for some reason will enable those values to be set. But that's obviously a hack, so the patch isn't wrong (because it makes it work).
I have no idea how to fix this, since that whole special key thing is part of kernelbase. Any thoughts?
Thanks for looking at this. The problem is that reg_create_key doesn't handle well creation of root-less keys. It shouldn't try to create "\Registry" key. The attached change helps here, does it work for you?
Thanks,
Jacek
Yeah it does, thanks for the fix, I'll resend v4 that checks for \\Registry\\ just in case (as is done in kernelbase).
participants (4)
-
Gabriel Ivăncescu -
Huw Davies -
Jacek Caban -
Marvin