Bug 169029

Summary: Leak under Options::setOptions
Product: WebKit Reporter: Tomas Popela <tpopela>
Component: JavaScriptCoreAssignee: Tomas Popela <tpopela>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, mcatanzaro, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 169055    
Bug Blocks: 104114    
Attachments:
Description Flags
Patch none

Tomas Popela
Reported 2017-03-01 04:06:50 PST
The optionsStrCopy variable is leaked in Options::setOptions in Source/JavaScriptCore/runtime/Options.cpp
Attachments
Patch (1.81 KB, patch)
2017-03-01 04:09 PST, Tomas Popela 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Tomas Popela
Comment 1 2017-03-01 04:09:12 PST
Created attachment 303061 [details] Patch
Michael Saboff
Comment 2 2017-03-01 07:35:03 PST
Comment on attachment 303061 [details] Patch r=me
Tomas Popela
Comment 3 2017-03-01 07:40:12 PST
Comment on attachment 303061 [details] Patch Clearing flags on attachment: 303061 Committed r213222: <http://trac.webkit.org/changeset/213222>
Tomas Popela
Comment 4 2017-03-01 07:40:21 PST
All reviewed patches have been landed. Closing bug.
Mark Lam
Comment 5 2017-03-01 09:59:22 PST
Comment on attachment 303061 [details] Patch This exposes a problem. Previously, by leaking the optionsStrCopy, we ensure that the option string being parsed by setOption() persists. Now that we free it properly, we can have a use after free scenario because we don't strdup string type options. See the FOR_EACH_OPTION macro in Options::setOptionWithoutAlias(). I think we should revert this patch and take the leak (since we don't parse options like this all the time) until we can fix string type options to strdup appropriately.
Alexey Proskuryakov
Comment 6 2017-03-01 13:32:06 PST
Yes, please roll out. A use after free is a much worse symptom.
Michael Saboff
Comment 7 2017-03-01 14:01:22 PST
I talked with Mark and I have a follow patch that fixes the UAF and reduces any leaks. I'll post in a few minutes. Filed <https://bugs.webkit.org/show_bug.cgi?id=169055> to track that change.
Note You need to log in before you can comment on or make changes to this bug.