Bug 134351

Summary: [Win] Fix potential buffer overrun in DLLLauncher
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Tools / TestsAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, bunhere, cdumez, commit-queue, ddkilzer, gyuyoung.kim, sergio
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Brent Fulgham 2014-06-26 10:44:41 PDT
Correct a possible buffer overrun in the DLLLauncher utility.
Comment 1 Brent Fulgham 2014-06-26 10:45:02 PDT
<rdar://problem/17469185>
Comment 2 Brent Fulgham 2014-06-26 10:46:41 PDT
Created attachment 233914 [details]
Patch
Comment 3 Brent Fulgham 2014-06-26 10:47:19 PDT
Created attachment 233915 [details]
Patch
Comment 4 Darin Adler 2014-06-26 11:41:15 PDT
Comment on attachment 233915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233915&action=review

> Tools/win/DLLLauncher/DLLLauncherMain.cpp:177
>      if (len >= bufSize)
> -        len = bufSize - 1;
> +        len = bufSize - 2;

This change looks wrong.

> Tools/win/DLLLauncher/DLLLauncherMain.cpp:179
>      errorMessage[len + 1] = 0;

This is the line of code that seems to have a bug. It should say errorMessage[len] = '\0';
Comment 5 Brent Fulgham 2014-06-26 16:54:17 PDT
Comment on attachment 233915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233915&action=review

>> Tools/win/DLLLauncher/DLLLauncherMain.cpp:179
>>      errorMessage[len + 1] = 0;
> 
> This is the line of code that seems to have a bug. It should say errorMessage[len] = '\0';

You are right. I just read the documents on FormatMessage, and it returns the number of characters NOT including null. So len + 1 was always wrong, as it gave at least one wchar_t's worth of garbage at the end of the line.
Comment 6 Brent Fulgham 2014-06-26 16:55:59 PDT
Created attachment 233946 [details]
Patch
Comment 7 Brent Fulgham 2014-06-26 17:04:26 PDT
Committed r170510: <http://trac.webkit.org/changeset/170510>