Re: kernel32: Tiny improvement to the GetVolumePathNameW stub (try 2)

[Michael Karcher]

Am Samstag, den 16.08.2008, 19:09 +0000 schrieb Louis. Lenders:
> Hi Michael, thanks for your comments. I've added some simple sanity
> checks from your first suggestions.
> It looks much better now, but you still access filename[1] before you
> even know that filename is not an empty string. In that case,
> filename[1] is past the end of the array, and you must not access it.
> Your check for strlenW comes too late. The minimal fix would be to put
> the strlenW
> call before the character comparisons, but it would be even smarter to
> either
>  a) check that filename[0] is not 0, and check the next characters in
>     order. So if any of these characters, short circuit evaluation
>     prevents access of later characters,
> or
>  b) just implement the special case for the empty string and catch
>     this case before you access filename[1] and filename[2].

Please think about what "sizeof(volumepathname)" will return. The
condition "sizeof(volumepathname) > 3" is *always* true. You should
check buflen at that point.
> As for the other suggestions, i'm sure that if this function ever gets
> implemented all these checks and tests are needed, but as i said, a
> bit of google for fixme:GetVolumePath shows the common way (the very
> few) apps call this function is with filename="c:\blabla\bla", so for
> now , in my opinion, i don't think it's worth the effort.

As long as there are no junctions in wine, your implementation is
correct for paths it can handle. So I would suggest to make the FIXME a
TRACE and add a FIXME("Can't handle path %s\n",debugstr_w(filename));
below the case you handle.

While googling for GetVolumeFileNameW, I stumbled across
 http://www.winehq.org/pipermail/wine-patches/2006-June/027228.html
This patch obviously has not been applied. That is not really
surprising, as it contains an extra hunk on an unrelated include file at
the end, and also ignores buflen, but it does contain the correct logic
for testing whether filename is a fully qualified local part, it
contains the GetVolumeFileNameA implementation that forwards to
GetVolumeFileNameW and contains at least one basic test.

I would suggest to polish that patch a bit and submit the extra goodies
from it also (you might use your GetPathNameW implementation after
fixing the points raised above; these implementations are mostly
equivalent). With polishing, I mean the following changes:
 - Test buflen in GetVolumeFileNameW (important)
 - The test "!filename[2]" is redundant (should be removed)
 (or just use your fixed version instead of the last two)
 - In the test: There is no need to allocate volumepath on the heap.
   char volumepath[16] (too bad you can't use constants in C for array
   dimensions!) would also do. Also, an ok(ret,"Error...") should
   precede the if(ret) block, the else part killed. If you expect
   something, always test it with an ok statement; do not sprinkle
   ok(FALSE) later in the code.

Regards,
  Michael Karcher