Closed Bug 1267187 Opened 4 years ago Closed 4 years ago

[Static Analysis][Division or modulo by zero] In function nsRuleNode::ComputeDisplayData

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1358669, CID 1358668, CID 1358667, CID 1358666, CID 1358665, CID 1358664, CID 1358663, CID 1358662, CID 1358661, CID 1358660, CID 1358659, CID 1358658 )

Attachments

(1 file)

The Static Analysis tool Coverity added a potentially modulo by zero can occur when member variable |num| from  TransitionPropData is used to determine the modulo of different variables for example:


>>    if (i >= animTimingFunction.num) {
>>      animation->SetTimingFunction(
>>        display->mAnimations[i % animTimingFunction.num].GetTimingFunction());
>>    }

In the above example |animTimingFunction| is a reference to:

>> TransitionPropData& animTimingFunction = animationPropData[3];

and array |animationPropData| is populated in function CountTransitionProps called in the context:
 
>>413  // CSS Animations.
>>
>>  uint32_t numAnimations =
>>    CountTransitionProps(animationPropInfo, animationPropData,
>>                         ArrayLength(animationPropData),
>>                         display, parentDisplay, aRuleData,
>>                         conditions);
>>
>>  display->mAnimations.SetLengthNonZero(numAnimations);

Looking through function CountTransitionProps i can't say for sure that |num| will not be 0, no matter the case is. So i think we should trigger an assert whenever modulo is done with |num| as the second argument, since in the event it's 0 the behavior will be undefined.
Attachment #8744853 - Flags: review?(cam) → review?(bbirtles)
Comment on attachment 8744853 [details]
MozReview Request: Bug 1267187 - added assert before doing modulo with the second argument being |num|. r?heycam

https://reviewboard.mozilla.org/r/48707/#review46641

I went through the code in question and I think we'll never fail these assertions because even when nothing is specified, we'll create a list for each of these properties with a single element corresponding to the initial value. That's also why the call to display->mAnimations.SetLengthNonZero(numAnimations) doesn't fail its assertion that numAnimations > 0.

::: layout/style/nsRuleNode.cpp:5370
(Diff revision 1)
>          transition->SetProperty(eCSSPropertyExtra_all_properties);
>        }
>      }
>  
>      if (i >= timingFunction.num) {
> +      MOZ_ASSERT(timingFunction.num, 

Nit: trailing whitespace

::: layout/style/nsRuleNode.cpp:5546
(Diff revision 1)
>        ComputeTimingFunction(animTimingFunction.list->mValue,
>                              animation->TimingFunctionSlot());
>      }
>  
>      if (i >= animDirection.num) {
> +      MOZ_ASSERT(animDirection.num, 

Nit: trailing whitespace

::: layout/style/nsRuleNode.cpp:5587
(Diff revision 1)
>        animation->SetFillMode(
>            static_cast<dom::FillMode>(animFillMode.list->mValue.GetIntValue()));
>      }
>  
>      if (i >= animPlayState.num) {
> +      MOZ_ASSERT(animPlayState.num, 

Nit: trailing whitespace

::: layout/style/nsRuleNode.cpp:5607
(Diff revision 1)
>  
>        animation->SetPlayState(animPlayState.list->mValue.GetIntValue());
>      }
>  
>      if (i >= animIterationCount.num) {
> +      MOZ_ASSERT(animIterationCount.num, 

Nit: trailing whitespace
Attachment #8744853 - Flags: review?(bbirtles) → review+
Comment on attachment 8744853 [details]
MozReview Request: Bug 1267187 - added assert before doing modulo with the second argument being |num|. r?heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48707/diff/1-2/
Attachment #8744853 - Flags: review?(cam)
https://hg.mozilla.org/mozilla-central/rev/8e9e288140e0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.