278306 - Possible unexpected truncation of path in GRE_GetCurrentProcessDirectory

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Wood (:Callek)]

DWORD GetModuleFileName(HMODULE hModule, LPTSTR lpFilename, DWORD nSize);

Return Values
If the function succeeds, the return value is the length of the string copied to
the buffer, in TCHARs. If the buffer is too small to hold the module name, the
string is truncated to nSize, and the function returns nSize.

If the function fails, the return value is zero. To get extended error
information, call GetLastError.


If the return value is > MAXPATHLEN here we should return PR_FALSE, better than
what we have now.

Patch Coming...

Comment 1

[Justin Wood (:Callek)]

Attachment: fix

This should fix it.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 171202 [details] [diff] [review]
fix

The documentation:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/g
etmodulefilename.asp

Does not talk about a return value greater than  MAXPATHLEN.  Why would be test
for it?  I would claim that GetModuleFileName would be broken if the return
value is greater than nSize.  Where do you see this happening?

Comment 3

[Darin Fisher]

Comment on attachment 171202 [details] [diff] [review]
fix

>Index: xpcom/glue/standalone/nsGREDirServiceProvider.cpp

>+    DWORD bufLength = ::GetModuleFileName(0, buffer, MAXPATHLEN);
>+    if (bufLength == 0 || bufLength >= MAXPATHLEN)
>+        return PR_FALSE;
>+    // chop of the executable name by finding the rightmost backslash
>+    char* lastSlash = PL_strrchr(buffer, '\\');

BTW, this should probably use _mbsrchr instead of PL_strrchr though
in practice I don't know of any character encodings where the current
code would be a problem (but in nsLocalFileWin.cpp we use _mbsrchr
when searching for '\\' so maybe there is a reason).

r+sr=darin with that change.

Comment 4

[Darin Fisher]

fixed-on-trunk

Comment 5

[Darin Fisher]

> Does not talk about a return value greater than  MAXPATHLEN.  Why would be test
> for it?  I would claim that GetModuleFileName would be broken if the return
> value is greater than nSize.  Where do you see this happening?

MSDN says: "If the buffer is too small to hold the module name, the string is
truncated to nSize, and the function returns nSize."  In this case MAXPATHLEN is
that nSize.  Given this documentation, it would have probably been fine to
compare using == instead of >=.

Sorry for not responding to your comment before checking in.  I didn't see it
when I first visited the bug.  Comments posted from the edit patch feature don't
handle conflict resolution IIRC :-(

Comment 6

[Justin Wood (:Callek)]

Attachment: Address Doug's Review comment

This fixes the >= to ==, for some reason I thought I read something like
"returns the size needed for the buffer" or some such, which of course would be
nicer since other app's could then allocate what is needed, O well.  Thanks

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 171225 [details] [diff] [review]
Address Doug's Review comment

yeah, that works.