[Bug 10649] New: Regression in RegQueryValueExA when called in unorthodox manner
http://bugs.winehq.org/show_bug.cgi?id=10649 Summary: Regression in RegQueryValueExA when called in unorthodox manner Product: Wine Version: CVS/GIT Platform: All OS/Version: All Status: UNCONFIRMED Severity: trivial Priority: P5 Component: wine-advapi32 AssignedTo: wine-bugs(a)winehq.org ReportedBy: samuel.howard.dennis(a)gmail.com commit bc590e87a6f9c7421ec3386a7c09a63a3e55dead (16/08/2006, Robert Shearman, affects advapi) caused a regression in one of my own programs in which I'd used an unusual calling convention for RegQueryValueEx, being this: char buf[16]; /* or 1 in the particular call that was failing */ DWORD count = sizeof buf; LONG ret; ret = RegQueryValueEx(hkey, "ValueName", NULL, &count, buf, &count); /* value left in count is never checked */ This works under real windows (9x at least, I never ran the program on installs of later Windows versions), but WINE does this before retrieving the value: if (type) *type = REG_NONE; ...which sets count to 0 since I pass the same address for both type and count in the call; this value is later used to determine the buffer size and triggers an overflow error. I am having trouble understanding the precise intent of the troublesome line (when is *type supposed to be set to REG_NONE? On any error? On any error other than buffer overflow? (This is the current WINE behaviour, as *type is unconditionally set again after copying the data)), but clearly assignments happen only after all processing in genuine Windows or *count is read early and that value is used throughout the function. I don't know which fix is appropriate, and am not sure how this case behaves across different versions of Windows so I'm submitting this bug instead of a patch. It is trivial to fix either way. There is also the issue of which value (type or count) is left in the single variable after the call, but calling this way and then checking that is even more perverse and nobody has probably ever done it. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 Zac Brown <zac(a)zacbrown.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |zac(a)zacbrown.org --- Comment #1 from Zac Brown <zac(a)zacbrown.org> 2007-12-02 15:44:30 --- This is an issue you should resolve in your own program, and is not something that should be fixed in wine in my opinion as our code reflects the expected in/out's of the method as defined on msdn(http://msdn2.microsoft.com/en-us/library/ms724911.aspx). If you read the above msdn doc, you'll see that the RegQueryValueEx function expects to have a count passed in but also a count AND type to pass back out. If you pass in the same memory location, of course you're going to end up with a buffer overrun the way its written in wine. My opinion is that you should call the function as intended rather than list this "undefined behavior" as a bug in wine. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #2 from James Hawkins <truiken(a)gmail.com> 2007-12-02 15:48:32 --- Zac, that's not how Wine works. msdn is not the be-all end-all concerning our implementation. If an app depends on an undefined behavior, we add a test case and implement it. Sam needs to add a unit test to our test suite, and assuming it passes on most versions of Windows, we'll fix it. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #3 from Zac Brown <zac(a)zacbrown.org> 2007-12-02 15:56:15 --- Ah I was unaware of that. In that case by all means! If MSDN is not the end-all-be-all then it should definitely be fixed. :) -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #4 from Sam Dennis <samuel.howard.dennis(a)gmail.com> 2007-12-02 17:21:24 --- Created an attachment (id=9466) --> (http://bugs.winehq.org/attachment.cgi?id=9466) Test case for problem It will work on Windows, the problem is that the early type setting change was presumably to match some undocumented behaviour too and not knowing exactly what that behaviour was, I can't test that. I've written a test case for this anyway (fairly messy, evidently my coding hasn't improved much since I wrote the gem in question), but the fix depends on what Windows returns in type for error cases other than a non-existent value. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #5 from Juan Lang <juan_lang(a)yahoo.com> 2007-12-03 13:18:32 --- Hi Sam, you should write the test case so that it passes on Windows (and fails on Wine) to show the problem. Mark the failing tests on Wine with todo_wine. I changed the last two ok calls in your patch to: + todo_wine ok(ret == ERROR_SUCCESS, "expected ERROR_SUCCESS, got %d\n", ret); + todo_wine ok(size == sizeof string1A, "%d (type?) returned when size expected - this is a false assumption if reported on Windows, the test case and WINE RegQueryValueEx should be fixed\n", size); and the tests succeed for me. Please make a similar change (and perhaps remove the "this is a false assumption..." bit, that's a tad verbose) and send to wine-patches. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #6 from Sam Dennis <samuel.howard.dennis(a)gmail.com> 2007-12-03 13:49:26 --- The problem with the second test is that I don't know what Windows does and can't test it. It could easily vary between versions. I'm not even sure that the first test will pass on all versions. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 Sam Dennis <samuel.howard.dennis(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9466 is|0 |1 obsolete| | --- Comment #7 from Sam Dennis <samuel.howard.dennis(a)gmail.com> 2007-12-05 12:33:37 --- Created an attachment (id=9502) --> (http://bugs.winehq.org/attachment.cgi?id=9502) Fix and test case I suppose I should just forget about the other aspects of the function's behaviour that I can't test and use the fix that's guaranteed not to affect anything else, which is reading the value early, although I suspect that it works on Windows because it's written late, if at all. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 Juan Lang <juan_lang(a)yahoo.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |juan_lang(a)yahoo.com --- Comment #8 from Juan Lang <juan_lang(a)yahoo.com> 2007-12-11 17:22:13 --- (In reply to comment #6)
The problem with the second test is that I don't know what Windows does and can't test it. It could easily vary between versions.
That's true. You can always ask for help testing patches on wine-devel. On your patch: - DWORD total_size; + DWORD total_size, datalen; (snip) + if (count) datalen = *count; (snip) - if (len > *count) status = STATUS_BUFFER_OVERFLOW; + if (len > datalen) status = STATUS_BUFFER_OVERFLOW; This can fail, datalen isn't initialized in all branches. You need to fix that. Same with the other two changes. Also, patches won't be picked up here - you'll want to send them to wine-patches. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #9 from Sam Dennis <samuel.howard.dennis(a)gmail.com> 2007-12-11 17:48:37 --- Actually, it is safe. It's never referenced unless data has a non-NULL value and if data is non-NULL and count is NULL, the function returns early with an error. I meant to send it to wine-patches but forgot, I'll do that now. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #10 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2007-12-12 03:30:35 --- You sent the patch to wine-patches with the same problems pointed out by Juan. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #11 from Sam Dennis <samuel.howard.dennis(a)gmail.com> 2007-12-12 10:30:46 --- As I explained, there is no problem. It's impossible to reach a reference to datalen without it having been initialised, only the most shallow of analysis would suggest otherwise (such as the ones compilers do). -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 --- Comment #12 from Juan Lang <juan_lang(a)yahoo.com> 2007-12-12 22:08:02 --- I just tried your test on WinXP, and it succeeds there. Also, size remains at 4 after the call, so you may want to add a test for that as well. Regarding your comment #4:
the problem is that the early type setting change was presumably to match some undocumented behaviour too and not knowing exactly what that behaviour was, I can't test that.
This is to match win9x when called for a non-existent value, as shown in the tests at line 570: http://source.winehq.org/source/dlls/advapi32/tests/registry.c#L570 It's possible to modify RegQueryValueExA only to set the type to 0 when the value isn't found/can't be read, which might be clearer than your approach. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 Juan Lang <juan_lang(a)yahoo.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution| |FIXED --- Comment #13 from Juan Lang <juan_lang(a)yahoo.com> 2007-12-14 12:39:14 --- This is now fixed (in 0.9.51.) Thanks, Sam. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 Dan Kegel <dank(a)kegel.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #14 from Dan Kegel <dank(a)kegel.com> 2008-01-28 05:45:49 --- Closing all RESOLVED FIXED bugs older than four weeks. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 Austin English <austinenglish(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|CVS/GIT |unspecified -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 Austin English <austinenglish(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Platform|All |Other OS/Version|All |other --- Comment #15 from Austin English <austinenglish(a)gmail.com> 2012-02-23 15:13:34 CST --- Removing deprecated 'All' Platform/OS. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=10649 Anastasius Focht <focht(a)gmx.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |1590b1f73198f34a5f49a177e66 | |c315aa0aa00a5 CC| |focht(a)gmx.net Version|unspecified |0.9.50. Regression SHA1| |bc590e87a6f9c7421ec3386a7c0 | |9a63a3e55dead -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
participants (1)
-
wine-bugs＠winehq.org