157875 – Modernize CSS code

RESOLVED FIXED 157875

Modernize CSS code

https://bugs.webkit.org/show_bug.cgi?id=157875

Summary Modernize CSS code

Alex Christensen

Reported 2016-05-18 17:43:48 PDT

Modernize CSS code

Attachments
Patch (31.30 KB, patch) 2016-05-18 17:45 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (33.41 KB, patch) 2016-05-19 13:24 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (34.03 KB, patch) 2016-05-19 15:09 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (39.82 KB, patch) 2016-05-23 17:15 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (40.18 KB, patch) 2016-05-23 21:15 PDT, Alex Christensen	cdumez: review+ cdumez: commit-queue-	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Alex Christensen

Comment 1 2016-05-18 17:45:28 PDT

Created attachment 279326 [details] Patch

Daniel Bates

Comment 2 2016-05-18 22:37:48 PDT

Comment on attachment 279326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279326&action=review The changes to CodeGeneratorJS.pm will break various bindings tests (we should teach the EWS to run the bindings tests to catch regressions). We need updated expected results. You can run the bindings tests by running Tools/Scripts/run-bindings-tests. > Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:56 > - return m_startValue->equals(*other.m_startValue.get()) && m_endValue->equals(*other.m_endValue.get()); > + if (!m_startValue->equals(other.m_startValue.get())) > + return false; > + if (!m_endValue && !other.m_endValue) > + return true; > + if (!m_endValue || !other.m_endValue) > + return false; > + return m_endValue->equals(*other.m_endValue.get()); :( I need to go to bed and have not reasoned about the correctness of this change. I take it that the original code was incorrect?

Alex Christensen

Comment 3 2016-05-18 22:56:33 PDT

Comment on attachment 279326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279326&action=review The rebased bindings tests are included in this patch. >> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:56 >> + return m_endValue->equals(*other.m_endValue.get()); > > :( I need to go to bed and have not reasoned about the correctness of this change. I take it that the original code was incorrect? The original code assumed that every CSSAnimationTriggerScrollValue had a non-null m_endValue, which is not guaranteed.

Alex Christensen

Comment 4 2016-05-19 00:55:21 PDT

I just wrote a crashing test that this patch fixes.

Alex Christensen

Comment 5 2016-05-19 00:55:56 PDT

<html> <head> <style> div { -webkit-animation-trigger : container-scroll(20px); } </style> <script> if (window.testRunner) testRunner.dumpAsText(); function run() { document.getElementById("test").focus(); document.execCommand('insertHTML', false, '<div id="insertedDiv" style="-webkit-animation-trigger : container-scroll(20px); "></div>'); } </script> </head> <body onload="run()"> <p>This test verifies that comparing two CSSAnimationTriggerScrollValues without end values does not crash.</p> <div id="test" contenteditable></div> </body> </html>

Alex Christensen

Comment 6 2016-05-19 00:57:32 PDT

Safari 9.1.1 is not vulnerable to this, but STP4 is.

Alex Christensen

Comment 7 2016-05-19 01:05:10 PDT

bug introduced in https://bugs.webkit.org/show_bug.cgi?id=142687

Simon Fraser (smfr)

Comment 8 2016-05-19 09:49:21 PDT

Comment on attachment 279326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279326&action=review > Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:55 > + if (!m_startValue->equals(other.m_startValue.get())) > + return false; > + if (!m_endValue && !other.m_endValue) > + return true; > + if (!m_endValue || !other.m_endValue) > + return false; Can this use arePointingToEqualData()? >>> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:56 >>> + return m_endValue->equals(*other.m_endValue.get()); >> >> :( I need to go to bed and have not reasoned about the correctness of this change. I take it that the original code was incorrect? > > The original code assumed that every CSSAnimationTriggerScrollValue had a non-null m_endValue, which is not guaranteed. If this is fixing a null deref, why is it a security bug?

Alex Christensen

Comment 9 2016-05-19 10:38:15 PDT

(In reply to comment #8) > Comment on attachment 279326 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279326&action=review > > > Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:55 > > + if (!m_startValue->equals(other.m_startValue.get())) > > + return false; > > + if (!m_endValue && !other.m_endValue) > > + return true; > > + if (!m_endValue || !other.m_endValue) > > + return false; > > Can this use arePointingToEqualData()? There's no operator==. That would look nicer, though, and it would save a deep comparison if they are pointing to the exact same CSSValue. > > >>> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:56 > >>> + return m_endValue->equals(*other.m_endValue.get()); > >> > >> :( I need to go to bed and have not reasoned about the correctness of this change. I take it that the original code was incorrect? > > > > The original code assumed that every CSSAnimationTriggerScrollValue had a non-null m_endValue, which is not guaranteed. > > If this is fixing a null deref, why is it a security bug? If it can crash the browser, isn't it a security bug?

Simon Fraser (smfr)

Comment 10 2016-05-19 10:40:44 PDT

(In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 279326 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=279326&action=review > > > > > Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:55 > > > + if (!m_startValue->equals(other.m_startValue.get())) > > > + return false; > > > + if (!m_endValue && !other.m_endValue) > > > + return true; > > > + if (!m_endValue || !other.m_endValue) > > > + return false; > > > > Can this use arePointingToEqualData()? > There's no operator==. That would look nicer, though, and it would save a > deep comparison if they are pointing to the exact same CSSValue. Can you add an operator ==? > > >>> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:56 > > >>> + return m_endValue->equals(*other.m_endValue.get()); > > >> > > >> :( I need to go to bed and have not reasoned about the correctness of this change. I take it that the original code was incorrect? > > > > > > The original code assumed that every CSSAnimationTriggerScrollValue had a non-null m_endValue, which is not guaranteed. > > > > If this is fixing a null deref, why is it a security bug? > If it can crash the browser, isn't it a security bug? No. Null derefs are not exploitable.

Said Abou-Hallawa

Comment 11 2016-05-19 11:01:44 PDT

Comment on attachment 279326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279326&action=review >>> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:55 >>> + return false; >> >> Can this use arePointingToEqualData()? > > There's no operator==. That would look nicer, though, and it would save a deep comparison if they are pointing to the exact same CSSValue. I would suggest writing the last two if-stamenets like this: if (!m_endValue || !other.m_endValue) return !m_endValue == !other.m_endValue; This statement says: if one endValue is nil, we can't do comparison expect checking the nullability for both.

Alex Christensen

Comment 12 2016-05-19 13:24:38 PDT

Created attachment 279421 [details] Patch

Alex Christensen

Comment 13 2016-05-19 15:09:42 PDT

Created attachment 279439 [details] Patch

Alex Christensen

Comment 14 2016-05-23 17:15:23 PDT

Created attachment 279609 [details] Patch

Chris Dumez

Comment 15 2016-05-23 18:40:11 PDT

Does not build on Windows and EFL?

Alex Christensen

Comment 16 2016-05-23 21:15:54 PDT

Created attachment 279621 [details] Patch

Chris Dumez

Comment 17 2016-05-23 22:02:59 PDT

Comment on attachment 279621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279621&action=review r=me > Source/WebCore/css/CSSCalculationValue.cpp:608 > + if (!value || !is<CSSPrimitiveValue>(value.get())) The null check is already included in is<CSSPrimitiveValue>() when passing a pointer, so your extra check is redundant. > Source/WebCore/css/CSSCalculationValue.cpp:611 > CSSPrimitiveValue& primitiveValue = downcast<CSSPrimitiveValue>(*value); The local variable does not bring much value. > Source/WebCore/css/CSSCalculationValue.cpp:612 > + result->value = CSSCalcPrimitiveValue::create(Ref<CSSPrimitiveValue>(primitiveValue), parserValue->isInt); I am assuming we need this because of the operator T() on CSSPrimitiveValue? We really need to fix that thing.

Alex Christensen

Comment 18 2016-05-23 22:34:30 PDT

http://trac.webkit.org/changeset/201318 (In reply to comment #17) > > Source/WebCore/css/CSSCalculationValue.cpp:612 > > + result->value = CSSCalcPrimitiveValue::create(Ref<CSSPrimitiveValue>(primitiveValue), parserValue->isInt); > > I am assuming we need this because of the operator T() on CSSPrimitiveValue? > We really need to fix that thing. Yep, this isn't the first time I've run into this. I didn't change it.

Alex Christensen

Comment 19 2016-05-24 13:46:44 PDT

Comment on attachment 279621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279621&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2921 > - push(@implContent, " RefPtr<${type}> forwardedImpl = castedThis->wrapped().${implGetterFunctionName}();\n"); > + push(@implContent, " auto forwardedImpl = castedThis->wrapped().${implGetterFunctionName}();\n"); This was bad. See https://bugs.webkit.org/show_bug.cgi?id=158037

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Alex Christensen

Reported

2016-05-18 17:43 PDT

Modified

2016-05-24 13:46 PDT History

CC List

4 users Show

URL

Keywords

Depends on

Blocks