[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: [e-mail]
        ReportedBy: [e-mail]

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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

Zac Brown  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Zac Brown   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #2 from James Hawkins   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #3 from Zac Brown   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. :)

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #4 from Sam Dennis   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #5 from Juan Lang   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #6 from Sam Dennis   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

Sam Dennis  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #9466 is|0                           |1
           obsolete|                            |

--- Comment #7 from Sam Dennis   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #8 from Juan Lang   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #9 from Sam Dennis   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #10 from Dmitry Timoshkov   2007-12-12 03:30:35 ---
You sent the patch to wine-patches with the same problems pointed out by Juan.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #11 from Sam Dennis   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).

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

--- Comment #12 from Juan Lang   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.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED

--- Comment #13 from Juan Lang   2007-12-14 12:39:14 ---
This is now fixed (in 0.9.51.)  Thanks, Sam.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

Dan Kegel  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #14 from Dan Kegel   2008-01-28 05:45:49 ---
Closing all RESOLVED FIXED bugs older than four weeks.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|CVS/GIT                     |unspecified

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Platform|All                         |Other
         OS/Version|All                         |other

--- Comment #15 from Austin English  2012-02-23 15:13:34 CST ---
Removing deprecated 'All' Platform/OS.

[Bug 10649] Regression in RegQueryValueExA when called in unorthodox manner

http://bugs.winehq.org/show_bug.cgi?id=10649

Anastasius Focht  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |1590b1f73198f34a5f49a177e66
                   |                            |c315aa0aa00a5
                 CC|                            |[e-mail]
            Version|unspecified                 |0.9.50.
    Regression SHA1|                            |bc590e87a6f9c7421ec3386a7c0
                   |                            |9a63a3e55dead