REGRESSION(277450@main): OOB array read with SVG animation where keyPoints = 0. #27481

mscottapple · 2024-04-18T23:18:40Z

`53e67f6`

REGRESSION(277450@main): OOB array read with SVG animation where keyPoints = 0.
https://bugs.webkit.org/show_bug.cgi?id=272929
rdar://126636733

Reviewed by Said Abou-Hallawa.

This change makes a couple additional, similar changes to the original changes
to better track the SVG spec. (See the original bug for more information.)

* LayoutTests/svg/animations/animate-zero-keyPoints-should-not-crash-expected.txt: Added.
* LayoutTests/svg/animations/animate-zero-keyPoints-should-not-crash.html: Added.
* Source/WebCore/svg/SVGAnimationElement.cpp:
(WebCore::SVGAnimationElement::keyTimes const):
(WebCore::SVGAnimationElement::startedActiveInterval):

Canonical link: https://commits.webkit.org/278212@main

c1a7f24

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-04-18T23:18:46Z

EWS run on previous version of this PR (hash bbc0bf7)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	⏳ 🧪 mac-wk1	✅ 🛠 wpe-skia
	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🛠 gtk
	~~🛠 tv~~	⏳ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~	✅ 🧪 api-gtk
	~~🛠 watch~~
	✅ 🛠 watch-sim

mscottapple · 2024-04-18T23:27:24Z

Added people involved with the change that introduced this crash.

webkit-early-warning-system · 2024-04-19T00:07:40Z

EWS run on previous version of this PR (hash c55c930)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-skia
	🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/svg/SVGAnimationElement.cpp

shallawa · 2024-04-19T01:10:18Z

Please include the blamed revision 277450@main in the title of the commit message.

webkit-early-warning-system · 2024-04-22T21:16:35Z

EWS run on previous version of this PR (hash 01ff56b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-04-23T17:55:18Z

EWS run on previous version of this PR (hash caa2678)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

...utTests/platform/gtk-wk2/svg/animations/animate-zero-keyPoints-should-not-crash-expected.txt

LayoutTests/svg/animations/animate-zero-keyPoints-should-not-crash.html

Source/WebCore/svg/SVGAnimationElement.cpp

webkit-early-warning-system · 2024-04-23T23:26:57Z

EWS run on previous version of this PR (hash a4cea47)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	✅ 🛠 wpe	~~🛠 wincairo~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 wpe-skia
	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🛠 gtk
	~~🛠 tv~~	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 gtk-wk2
	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~	⏳ 🧪 api-gtk
	~~🛠 watch~~
	~~🛠 watch-sim~~

Source/WebCore/svg/SVGAnimationElement.cpp

webkit-early-warning-system · 2024-04-24T00:09:34Z

EWS run on previous version of this PR (hash 7b6d1a4)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

shallawa · 2024-04-24T00:34:07Z

Please change the commit message to describe correctly the current change.

mscottapple · 2024-04-24T17:10:15Z

Now the test imported/w3c/web-platform-tests/svg/animations/animateMotion-keyPoints-001.html is failing with assert_equals: y position expected 0 but got -50

shallawa · 2024-04-24T19:50:15Z

I think the specs is clear about the animateMotion overriding rules for the motion path and the overriding rules for the keyPoints.

But I think it is not clear about the overriding rules for the keyTimes when the calcMode is Paced and the animationMode is Path. When calcMode is Paced we usually use the values attribute to generate interpolated values for keyTimes. But with animationMode equal to Path I think values should be ignored in this case. And I would expect if calcMode is Paced and animationMode is Path is to have the attribute keyTimes specified in the <animateMotion> tag.

This is consistent with our code. In SVGAnimationElement::calculatePercentFromKeyPoints() we use m_keyTimesFromAttribute and not m_keyTimesForPaced as the working keyTimes. Look for where calculatePercentFromKeyPoints() is called. You will find it is called only if (calcMode != CalcMode::Paced || animationMode == AnimationMode::Path)

So to fix this issue, you will need to add this change to your patch.

const Vector<float>& SVGAnimationElement::keyTimes() const
{
    return calcMode() != CalcMode::Paced || animationMode() == AnimationMode::Path ? m_keyTimesFromAttribute : m_keyTimesForPaced;
}

SupaYo · 2024-04-24T19:55:12Z

What do I have to do to fix the problem?

webkit-early-warning-system · 2024-04-30T19:42:30Z

EWS run on previous version of this PR (hash cd30bd5)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	⏳ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2-stress	⏳ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-04-30T22:51:40Z

EWS run on current version of this PR (hash c1a7f24)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

shallawa · 2024-05-01T06:04:32Z

Source/WebCore/svg/SVGAnimationElement.cpp

Nit: There is no need for the parentheses. The operator precedence should work as expected:

The equality != and ==

The logical &&

The ternary conditional ? :

…oints = 0. https://bugs.webkit.org/show_bug.cgi?id=272929 rdar://126636733 Reviewed by Said Abou-Hallawa. This change makes a couple additional, similar changes to the original changes to better track the SVG spec. (See the original bug for more information.) * LayoutTests/svg/animations/animate-zero-keyPoints-should-not-crash-expected.txt: Added. * LayoutTests/svg/animations/animate-zero-keyPoints-should-not-crash.html: Added. * Source/WebCore/svg/SVGAnimationElement.cpp: (WebCore::SVGAnimationElement::keyTimes const): (WebCore::SVGAnimationElement::startedActiveInterval): Canonical link: https://commits.webkit.org/278212@main

webkit-commit-queue · 2024-05-01T17:08:29Z

Committed 278212@main (53e67f6): https://commits.webkit.org/278212@main

Reviewed commits have been landed. Closing PR #27481 and removing active labels.

mscottapple self-assigned this Apr 18, 2024

mscottapple added the SVG For bugs in the SVG implementation. label Apr 18, 2024

mscottapple requested review from anttijk, graouts and grorg April 18, 2024 23:25

mscottapple force-pushed the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch from bbc0bf7 to c55c930 Compare April 19, 2024 00:05

shallawa reviewed Apr 19, 2024

View reviewed changes

Source/WebCore/svg/SVGAnimationElement.cpp Outdated Show resolved Hide resolved

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 19, 2024

mscottapple removed the merging-blocked Applied to prevent a change from being merged label Apr 22, 2024

mscottapple changed the title ~~Fix an OOB array read with SVG animation where keyPoints = 0.~~ REGRESSION(277450@main): OOB array read with SVG animation where keyPoints = 0. Apr 22, 2024

mscottapple force-pushed the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch from c55c930 to 01ff56b Compare April 22, 2024 21:14

mscottapple requested a review from shallawa April 23, 2024 00:06

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 23, 2024

mscottapple removed the merging-blocked Applied to prevent a change from being merged label Apr 23, 2024

mscottapple force-pushed the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch from 01ff56b to caa2678 Compare April 23, 2024 17:55

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 23, 2024

shallawa requested changes Apr 23, 2024

View reviewed changes

mscottapple removed the merging-blocked Applied to prevent a change from being merged label Apr 23, 2024

mscottapple force-pushed the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch from caa2678 to a4cea47 Compare April 23, 2024 23:26

mscottapple requested a review from shallawa April 23, 2024 23:30

shallawa reviewed Apr 23, 2024

View reviewed changes

Source/WebCore/svg/SVGAnimationElement.cpp Outdated Show resolved Hide resolved

mscottapple force-pushed the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch from a4cea47 to 7b6d1a4 Compare April 24, 2024 00:09

mscottapple requested a review from shallawa April 24, 2024 00:21

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 24, 2024

mscottapple removed the merging-blocked Applied to prevent a change from being merged label Apr 30, 2024

mscottapple force-pushed the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch from 7b6d1a4 to cd30bd5 Compare April 30, 2024 19:42

mscottapple force-pushed the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch from cd30bd5 to c1a7f24 Compare April 30, 2024 22:48

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 1, 2024

shallawa approved these changes May 1, 2024

View reviewed changes

shallawa added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels May 1, 2024

webkit-commit-queue force-pushed the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch from c1a7f24 to 53e67f6 Compare May 1, 2024 17:08

webkit-commit-queue merged commit 53e67f6 into WebKit:main May 1, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 1, 2024

mscottapple deleted the eng/OOB-array-read-with-SVG-animation-where-keyPoints--0 branch May 1, 2024 17:17

REGRESSION(277450@main): OOB array read with SVG animation where keyPoints = 0. #27481

REGRESSION(277450@main): OOB array read with SVG animation where keyPoints = 0. #27481

Uh oh!

Conversation

mscottapple commented Apr 18, 2024 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

53e67f6

Uh oh!

webkit-early-warning-system commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscottapple commented Apr 18, 2024

Uh oh!

webkit-early-warning-system commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shallawa commented Apr 19, 2024

Uh oh!

webkit-early-warning-system commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webkit-early-warning-system commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

webkit-early-warning-system commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shallawa commented Apr 24, 2024

Uh oh!

mscottapple commented Apr 24, 2024

Uh oh!

shallawa commented Apr 24, 2024

Uh oh!

SupaYo commented Apr 24, 2024

Uh oh!

webkit-early-warning-system commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shallawa May 1, 2024

Choose a reason for hiding this comment

Uh oh!

webkit-commit-queue commented May 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mscottapple commented Apr 18, 2024 •

edited by webkit-early-warning-system

Loading

`53e67f6`

webkit-early-warning-system commented Apr 18, 2024 •

edited

Loading

webkit-early-warning-system commented Apr 19, 2024 •

edited

Loading

webkit-early-warning-system commented Apr 22, 2024 •

edited

Loading

webkit-early-warning-system commented Apr 23, 2024 •

edited

Loading

webkit-early-warning-system commented Apr 23, 2024 •

edited

Loading

webkit-early-warning-system commented Apr 24, 2024 •

edited

Loading

webkit-early-warning-system commented Apr 30, 2024 •

edited

Loading

webkit-early-warning-system commented Apr 30, 2024 •

edited

Loading