Bug 210353

Summary: Null ptr Deref in RadioButtonGroups::updateCheckedState
Product: WebKit Reporter: Pinki Gyanchandani <pgyanchandani>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, esprehn+autocc, ews-watchlist, kangil.han, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Pinki Gyanchandani
Reported 2020-04-10 13:27:22 PDT
Crash is observed when default checked is called for HTML Input element and radioButtonGroup is NULL.
Attachments
Patch (3.62 KB, patch)
2020-04-10 13:37 PDT, Pinki Gyanchandani 		no flags
DetailsFormatted DiffDiff
Patch (3.58 KB, patch)
2020-04-10 14:26 PDT, Pinki Gyanchandani 		no flags
DetailsFormatted DiffDiff
Patch (3.59 KB, patch)
2020-04-10 15:45 PDT, Pinki Gyanchandani 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Pinki Gyanchandani
Comment 1 2020-04-10 13:29:25 PDT
rdar://problem/61290022
Pinki Gyanchandani
Comment 2 2020-04-10 13:37:51 PDT
Created attachment 396118 [details] Patch
Wenson Hsieh
Comment 3 2020-04-10 13:44:32 PDT
Comment on attachment 396118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396118&action=review > Source/WebCore/ChangeLog:8 > + Crash happend when default checked setter was called an input element and RadioButtonGroup was NULL. Nit - “This crash happened when the default…" > Source/WebCore/ChangeLog:9 > + Added a NULL pointer check before derefercing. Nit - “dereferencing" > LayoutTests/fast/forms/input-element-default-checked-setter-crash.html:1 > +<!DOCHTML> Nit - <!DOCTYPE html>?
Wenson Hsieh
Comment 4 2020-04-10 13:52:40 PDT
Comment on attachment 396118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396118&action=review > LayoutTests/fast/forms/input-element-default-checked-setter-crash.html:16 > +<meta id="meta1"> </meta> I don’t think this meta tag is necessary to reproduce this crash. Here’s a slightly simpler version of this test case: <!DOCTYPE html> <html> <body> <p>This test passes if there is no crash</p> <div id="container"> <iframe id="frame"></iframe> <input id="input" name=" " type="radio"> </div> <script> if (window.testRunner) testRunner.dumpAsText(); frame.onload = () => input.defaultChecked = true; document.body.appendChild(container); </script> </body> </html>
Chris Dumez
Comment 5 2020-04-10 13:54:40 PDT
Comment on attachment 396118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396118&action=review > Source/WebCore/dom/RadioButtonGroups.cpp:231 > + auto* group = m_nameToGroupMap.get(element.name().impl()); if (auto* group = m_nameToGroupMap.get(element.name().impl())) group->updateCheckedState(element) would be more concise.
Pinki Gyanchandani
Comment 6 2020-04-10 14:26:42 PDT
Created attachment 396125 [details] Patch
Pinki Gyanchandani
Comment 7 2020-04-10 14:27:59 PDT
Hi Chirs and Wensen, Thanks for quick review comments. I have incorporated the comments. Regards, Pinki
Chris Dumez
Comment 8 2020-04-10 14:29:59 PDT
Comment on attachment 396125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396125&action=review > Source/WebCore/ChangeLog:8 > + This crash happend when the default checked setter was called for an input element and RadioButtonGroup was NULL. typo: happend > Source/WebCore/ChangeLog:9 > + Added condition to deference the group only if it is valid. typo: deference -> dereference s/valid/non-null > LayoutTests/fast/forms/input-element-default-checked-setter-crash.html:4 > +<p>This test passes if there is no crash</p> nit: There should be a period at the end of your sentence.
Pinki Gyanchandani
Comment 9 2020-04-10 15:45:31 PDT
Created attachment 396131 [details] Patch
EWS
Comment 10 2020-04-10 16:16:01 PDT
Committed r259911: <https://trac.webkit.org/changeset/259911> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396131 [details].
Note You need to log in before you can comment on or make changes to this bug.