1333418 - Array bounds crash [@ BuildSegmentsFromValueEntries]

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase crashes in mozilla-central rev 8ff550409e.
It also crashes inbound rev cdc8e1a140e2 with the patches from 1331704.

==25570==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f3a0b1744be bp 0x7ffe451438f0 sp 0x7ffe451438d0 T0)
    #0 0x7f3a0b1744bd in InvalidArrayIndex_CRASH(unsigned long, unsigned long) /home/truber/src/m/unified/xpcom/glue/nsTArray.cpp:35:3
    #1 0x7f3a0d6e993b in ElementAt /home/truber/src/m/unified/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1160:7
    #2 0x7f3a0d6e993b in operator[] /home/truber/src/m/unified/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1198
    #3 0x7f3a0d6e993b in BuildSegmentsFromValueEntries /home/truber/src/m/unified/dom/animation/KeyframeUtils.cpp:1350
    #4 0x7f3a0d6e993b in mozilla::KeyframeUtils::GetAnimationPropertiesFromKeyframes(nsTArray<mozilla::Keyframe> const&, nsTArray<nsTArray<mozilla::PropertyStyleAnimationValuePair> > const&, mozilla::dom::CompositeOperation, nsStyleContext*) /home/truber/src/m/unified/dom/animation/KeyframeUtils.cpp:715
    #5 0x7f3a0d6cc98b in mozilla::dom::KeyframeEffectReadOnly::BuildProperties(nsStyleContext*) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:860:5
    #6 0x7f3a0d6baba2 in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties(nsStyleContext*) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:286:44
    #7 0x7f3a0d6cc2e8 in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(nsTArray<mozilla::Keyframe>&&, nsStyleContext*) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:208:5
    #8 0x7f3a0d6cbb35 in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(JSContext*, JS::Handle<JSObject*>, mozilla::ErrorResult&) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:185:3
    #9 0x7f3a0d6c3e98 in already_AddRefed<mozilla::dom::KeyframeEffect> mozilla::dom::KeyframeEffectReadOnly::ConstructKeyframeEffect<mozilla::dom::KeyframeEffect, mozilla::dom::UnrestrictedDoubleOrKeyframeEffectOptions>(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeEffectOptions const&, mozilla::ErrorResult&) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:784:3
    #10 0x7f3a0d6c346d in mozilla::dom::KeyframeEffect::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeEffectOptions const&, mozilla::ErrorResult&) /home/truber/src/m/unified/dom/animation/KeyframeEffect.cpp:45:10
    #11 0x7f3a0f5003b1 in mozilla::dom::KeyframeEffectBinding::_constructor(JSContext*, unsigned int, JS::Value*) /home/truber/src/m/unified/obj-x86_64-pc-linux-gnu/dom/bindings/KeyframeEffectBinding.cpp:1815:64

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Regression from bug 1305325.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

OK. 'spacing: paced(perspactive)' computes computed offsets as 0 in the second keyframe.
After ApplySpacing, the keyframes are:

 { perspective: 'none',           width: 'auto', computedOffset: 0 }
 { perspective: '172.17866832in', width: 'auto', computedOffset: 0 }
 { perspective: 0,                               computedOffset: 1 }

Boris, I need your help.
Is this expected behavior?  If so, we need to handle the case in BuildSegmentsFromValueEntries().

Comment 3

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> OK. 'spacing: paced(perspactive)' computes computed offsets as 0 in the
> second keyframe.
> After ApplySpacing, the keyframes are:
> 
>  { perspective: 'none',           width: 'auto', computedOffset: 0 }
>  { perspective: '172.17866832in', width: 'auto', computedOffset: 0 }
>  { perspective: 0,                               computedOffset: 1 }
> 
> Boris, I need your help.
> Is this expected behavior?  If so, we need to handle the case in
> BuildSegmentsFromValueEntries().

Yes, I think so. We treat zero perspective as inf perspective [1], so the 3rd keyframe is infinite perspective, so the 2nd computedOffset becomes 0.

[1] http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/layout/style/nsStyleTransformMatrix.h#32-41

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thank you Boris.

I just checked the case again.

I think 'j + 1 < n' check is necessary at [1] like we do just below another while().

[1] https://hg.mozilla.org/mozilla-central/file/6dccae211ae5/dom/animation/KeyframeUtils.cpp#l1350

Comment 5

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62c018904beb94830b3f8d8c16ebaff30dfc3e3e

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Don't exceed index of KeyframeValueEntry more than entry's length

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8830240 [details]
Don't exceed index of KeyframeValueEntry more than entry's length

Gosh, why didn't I use MozReview?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length.

Review commit: https://reviewboard.mozilla.org/r/107136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107136/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8830242 [details]
Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length.

https://reviewboard.mozilla.org/r/107136/#review108436

::: dom/animation/test/chrome/test_animation_properties.html:821
(Diff revision 1)
> +  { desc:     'a missing property in initial keyframe with duplicate offset '
> +              + 'along with other values',
> +    frames:   [ { left: '10px',              offset: 0 },
> +                { left: '8px', right: '8px', offset: 1 },
> +                { left: '5px', right: '5px', offset: 1 } ],
> +    expected: [ { property: 'left',
> +                  values: [ value(0, '10px', 'replace', 'linear'),
> +                            value(1, '8px',  'replace'),
> +                            value(1, '5px',  'replace') ] },
> +                { property: 'right',
> +                  values: [ value(0, undefined, 'add', 'linear'),

This second test case don't hit this bug at all because this case is processed another while loop which has boundary check[1]

[1] https://hg.mozilla.org/mozilla-central/file/24d9eb148461/dom/animation/KeyframeUtils.cpp#l1359

The first case of course hits this bug.

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8830242 [details]
Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length.

https://reviewboard.mozilla.org/r/107136/#review109174

::: dom/animation/test/chrome/test_animation_properties.html:806
(Diff revision 1)
> +  { desc:     'a missing property in final keyframe with duplicate offset along'
> +              + 'with other values',

Nit: Missing a space between 'along' and 'with'

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8830242 [details]
Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107136/diff/1-2/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df653a73e414
Don't exceed index of KeyframeValueEntry more than entry's length. r=birtles

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df653a73e414

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8830242 [details]
Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1305325.
[User impact if declined]: Crash.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet at this point, just landed on mozilla-central.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: I don't think so.
[Why is the change risky/not risky?]: Because the change just adds a boundary check.
[String changes made/needed]: None.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8830242 [details]
Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length.

Crash fix, Aurora53+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb0502e9fcd7