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