Modernize CSS code
Created attachment 279326 [details] Patch
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?
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.
I just wrote a crashing test that this patch fixes.
<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>
Safari 9.1.1 is not vulnerable to this, but STP4 is.
bug introduced in https://bugs.webkit.org/show_bug.cgi?id=142687
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?
(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?
(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.
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.
Created attachment 279421 [details] Patch
Created attachment 279439 [details] Patch
Created attachment 279609 [details] Patch
Does not build on Windows and EFL?
Created attachment 279621 [details] Patch
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.
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.
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