Bug 27381 – WinLauncher Crash with File URLs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

Attempting to open a file URL in WinLauncher (e.g., C:\Cygwin\tmp\layout-test-results\results.html) will result in a crash in FastAlloc.  This is happening because it is trying to allocate an enormous number of bytes (e.g., 174266262).

The problem is caused by this section of code:

         BSTR urlBstr = ...

         TCHAR fileURL[INTERNET_MAX_URL_LENGTH];
         DWORD fileURLLength = sizeof(fileURL)/sizeof(fileURL[0]);
         if (SUCCEEDED(UrlCreateFromPath(urlBStr, fileURL, &fileURLLength, 0)))
              urlBStr = fileURL;

It is attempting to assign a TCHAR (generally a UNICODE string) to a BSTR.  While the compiler allows this, the BSTR looses the size value that should be prepended to the string.  Later on, in MarshallingHelpers.cpp we attempt this code:

KURL MarshallingHelpers::BSTRToKURL(BSTR urlStr)
{
    return KURL(KURL(), String(urlStr, SysStringLen(urlStr)));
}

The call to SysStringLen attempts to interpret the first four bytes of the UNICODE value as a string length, which in the test case results in an size that is larger than available memory.

Fix is attached.

[Brent Fulgham]

Created attachment 32960 [details]
Correct BSTR mishandling.

[Adam Roben (:aroben)]

Comment on attachment 32960 [details]
Correct BSTR mishandling.

>          if (SUCCEEDED(UrlCreateFromPath(urlBStr, fileURL, &fileURLLength, 0)))
> -            urlBStr = fileURL;
> +            SysReAllocString(&urlBStr, fileURL);

It doesn't seem so great to be modifying the urlBStr parameter like this. Maybe it would be better to put the file: URL into a separate variable? Something like:

BSTR fileURLBstr = 0;
if (...) {
    fileURLBstr = SysAllocString(...);
    urlBStr = fileURLBstr;
}

...actual loading calls here...

if (fileURLBstr)
    SysFreeString(fileURLBstr);

But it seems OK as-is.

[Brent Fulgham]

Landed in @r46050.

[Brent Fulgham]

Forgot to close issue.