710971 - Possible bad logic / type in nsStyleAnimation::AddWeighted()

Status: NEW

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Justin Dolske [:Dolske]]

From http://www.viva64.com/en/a/0078/

Example 5. Incomplete checking of input values

PRBool
nsStyleAnimation::AddWeighted(...)
{
  ...
  if (unit[0] == eCSSUnit_Null || unit[1] == eCSSUnit_Null ||
      unit[0] == eCSSUnit_Null || unit[0] == eCSSUnit_URL) {
    return PR_FALSE;
  }
  ...
}

PVS-Studio diagnostic message: V501 There are identical sub-expressions 'unit [0] == eCSSUnit_Null' to the left and to the right of the '||' operator. nsstyleanimation.cpp 1767

It seems to me that this code fragment contains 2 misprints simultaneously. I cannot tell for sure how exactly the code should look, but the developers most likely intended it to be written as follows:

if (unit[0] == eCSSUnit_Null || unit[1] == eCSSUnit_Null ||
    unit[0] == eCSSUnit_URL  || unit[1] == eCSSUnit_URL) {

The misprints may cause the function to process incorrect input values.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We need to look at both ComputeDistance and AddWeighted.

There was originally only an nsCSSValuePair case, and it checked both units against null.

https://hg.mozilla.org/mozilla-central/rev/dd88bfc07421 added a URL check for the first unit.  Sort of a special case, but reasonable.

I think the mistakes come in the CSSValueTriplet case, from https://hg.mozilla.org/mozilla-central/rev/ea882f18d8ac :
 * ComputeDistance and AddWeighted have different logic there
 * neither checks unit[2] against eCSSUnit_Null, though ComputeDistance almost did (triggering this warning)
 * I think, additionally, neither ValueTriplet case should check against eCSSUnit_URL

Nothing actually looks dangerous here, though.