Coverity defects for nsCSSParser.cpp

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

Filing this bug to cover all of the Coverity defects that were recently reported for nsCSSParser.cpp (via an email that jesup forwarded to me).

I've included my analysis/thoughts below.  Happily, none of the errors are real issues; though I think we can & should suppress a few with "mozilla::unused".

> *** CID 1401415:  Error handling issues  (CHECKED_RETURN)
> /layout/style/nsCSSParser.cpp: 4089 in <unnamed>::CSSParserImpl::ParseFontDescriptor(nsCSSFontFaceRule *)()
> 4083     
> 4084       if (descID == eCSSFontDesc_UNKNOWN ||
> 4085           (descID == eCSSFontDesc_Display &&
> 4086            !Preferences::GetBool("layout.css.font-display.enabled"))) {
> 4087         if (NonMozillaVendorIdentifier(descName)) {
> 4088           // silently skip other vendors' extensions
> >>>     CID 1401415:  Error handling issues  (CHECKED_RETURN)
> >>>     Calling "SkipDeclaration" without checking return value (as is done elsewhere 9 out of 10 times).
> 4089           SkipDeclaration(true);
> 4090           return true;

This is for parsing the contents of a @font-face {...} rule.

This code is fine because:
 (a) The goal of this SkipDeclaration call is to explicitly skip past a "-webkit-something: something;" declaration.
 (b) SkipDeclaration() only fails if it hits the end-of-input (if there are no more tokens)
 (c) If that happens, then the caller will detect that anyway (in its next !GetToken call) in the CSSParserImpl::ParseFontFaceRule loop, and it will report the same error as it would've if there were no "-webkit-something" declaration, which is appropriate.

SUGGESTED FIX: Use mozilla::unused to suppress this Coverity report (and to make it more explicit that we're intentionally ignoring the return val).


> *** CID 1401416:    (CHECKED_RETURN)
> /layout/style/nsCSSParser.cpp: 17061 in <unnamed>::CSSParserImpl::ParseShadowItem(nsCSSValue &, bool)()
> 17055       };
> 17056     
> 17057       RefPtr<nsCSSValue::Array> val = nsCSSValue::Array::Create(6);
> 17058     
> 17059       if (aIsBoxShadow) {
> 17060         // Optional inset keyword (ignore errors)
> >>>     CID 1401416:    (CHECKED_RETURN)
> >>>     Calling "ParseSingleTokenVariant" without checking return value (as is done elsewhere 121 out of 130 times).
> 17061         ParseSingleTokenVariant(val->Item(IndexInset), VARIANT_KEYWORD,
> 17062                                 nsCSSProps::kBoxShadowTypeKTable);
> 17063       }
> 17064     
> 17065       nsCSSValue xOrColor;
> 17066       bool haveColor = false;
> /layout/style/nsCSSParser.cpp: 17129 in <unnamed>::CSSParserImpl::ParseShadowItem(nsCSSValue &, bool)()
> 17123           return false;
> 17124         }
> 17125       }
> 17126     
> 17127       if (aIsBoxShadow && val->Item(IndexInset).GetUnit() == eCSSUnit_Null) {
> 17128         // Optional inset keyword
> >>>     CID 1401416:    (CHECKED_RETURN)
> >>>     Calling "ParseSingleTokenVariant" without checking return value (as is done elsewhere 121 out of 130 times).
> 17129         ParseSingleTokenVariant(val->Item(IndexInset), VARIANT_KEYWORD,
> 17130                                 nsCSSProps::kBoxShadowTypeKTable);
> 17131       }

Both of these seem to be fine -- they have a comment saying "optional" and the first one even says "ignore errors".

SUGGESTED FIX: Use mozilla::unused to suppress this Coverity report (and to make it more explicit that we're intentionally ignoring the return val).

> *** CID 1401417:    (CHECKED_RETURN)
> /layout/style/nsCSSParser.cpp: 15494 in <unnamed>::CSSParserImpl::ParseTextEmphasisStyle(nsCSSValue &)()
> 15488       }
> 15489     
> 15490       nsCSSValue first, second;
> 15491       const auto& fillKTable = nsCSSProps::kTextEmphasisStyleFillKTable;
> 15492       const auto& shapeKTable = nsCSSProps::kTextEmphasisStyleShapeKTable;
> 15493       if (ParseSingleTokenVariant(first, VARIANT_KEYWORD, fillKTable)) {
> >>>     CID 1401417:    (CHECKED_RETURN)
> >>>     Calling "ParseSingleTokenVariant" without checking return value (as is done elsewhere 121 out of 130 times).
> 15494         ParseSingleTokenVariant(second, VARIANT_KEYWORD, shapeKTable);
> 15495       } else if (ParseSingleTokenVariant(first, VARIANT_KEYWORD, shapeKTable)) {
> 15496         ParseSingleTokenVariant(second, VARIANT_KEYWORD, fillKTable);
> 15497       } else {
> 15498         return false;
> 15499       }
> /layout/style/nsCSSParser.cpp: 15496 in <unnamed>::CSSParserImpl::ParseTextEmphasisStyle(nsCSSValue &)()
> 15490       nsCSSValue first, second;
> 15491       const auto& fillKTable = nsCSSProps::kTextEmphasisStyleFillKTable;
> 15492       const auto& shapeKTable = nsCSSProps::kTextEmphasisStyleShapeKTable;
> 15493       if (ParseSingleTokenVariant(first, VARIANT_KEYWORD, fillKTable)) {
> 15494         ParseSingleTokenVariant(second, VARIANT_KEYWORD, shapeKTable);
> 15495       } else if (ParseSingleTokenVariant(first, VARIANT_KEYWORD, shapeKTable)) {
> >>>     CID 1401417:    (CHECKED_RETURN)
> >>>     Calling "ParseSingleTokenVariant" without checking return value (as is done elsewhere 121 out of 130 times).
> 15496         ParseSingleTokenVariant(second, VARIANT_KEYWORD, fillKTable);
> 15497       } else {
> 15498         return false;
> 15499       }


Same here.

> *** CID 1401418:  Incorrect expression  (COPY_PASTE_ERROR)
> /layout/style/nsCSSParser.cpp: 9748 in <unnamed>::CSSParserImpl::ParseGridColumnRow(nsCSSPropertyID, nsCSSPropertyID)()
> 9742       if (!ParseGridLine(value)) {
> 9743         return false;
> 9744       }
> 9745       if (GetToken(true)) {
> 9746         if (mToken.IsSymbol('/')) {
> 9747           if (ParseGridLine(secondValue)) {
> >>>     CID 1401418:  Incorrect expression  (COPY_PASTE_ERROR)
> >>>     "value" looks like a copy-paste error.
> 9748             AppendValue(aStartPropID, value);
> 9749             AppendValue(aEndPropID, secondValue);
> 9750             return true;

This is just Coverity mistakenly applying one of its heuristics.  "value" seems likely-correct here.

(Coverity is probably confused, because we *just parsed* secondValue, and then
we do something with "value" instead of with "secondValue".  But the point is
that we accept them *as a pair*, once we've parsed both of them.)

> *** CID 1401422:  Control flow issues  (UNREACHABLE)
> /layout/style/nsCSSParser.cpp: 8243 in <unnamed>::CSSParserImpl::ParseImageRect(nsCSSValue &)()
> 8237      * -moz-image-rect(<uri>, <top>, <right>, <bottom>, <left>)
> 8238      */
> 8239     bool
> 8240     CSSParserImpl::ParseImageRect(nsCSSValue& aImage)
> 8241     {
> 8242       // A non-iterative for loop to break out when an error occurs.
> >>>     CID 1401422:  Control flow issues  (UNREACHABLE)
> >>>     Since the loop increment is unreachable, the loop body will never execute more than once.
> 8243       for (;;) {
> 8244         nsCSSValue newFunction;
> 8245         static const uint32_t kNumArgs = 5;

This is Coverity being overly-concerned. It's *intentional* that the loop body
only executes once, per the "non-iterative for loop" code-comment.  This is
just how we group all of the error-handling together here.

> *** CID 1401423:  Control flow issues  (UNREACHABLE)
> /layout/style/nsCSSParser.cpp: 8006 in <unnamed>::CSSParserImpl::ParseCounter(nsCSSValue &)()
> 8000     CSSParserImpl::ParseCounter(nsCSSValue& aValue)
> 8001     {
> 8002       nsCSSUnit unit = (mToken.mIdent.LowerCaseEqualsLiteral("counter") ?
> 8003                         eCSSUnit_Counter : eCSSUnit_Counters);
> 8004     
> 8005       // A non-iterative for loop to break out when an error occurs.
> >>>     CID 1401423:  Control flow issues  (UNREACHABLE)
> >>>     Since the loop increment is unreachable, the loop body will never execute more than once.
> 8006       for (;;) {

Here, too.

> *** CID 1401424:  Control flow issues  (UNREACHABLE)
> /layout/style/nsCSSParser.cpp: 8287 in <unnamed>::CSSParserImpl::ParseElement(nsCSSValue &)()
> 8281     
> 8282     // <element>: -moz-element(# <element_id> )
> 8283     bool
> 8284     CSSParserImpl::ParseElement(nsCSSValue& aValue)
> 8285     {
> 8286       // A non-iterative for loop to break out when an error occurs.
> >>>     CID 1401424:  Control flow issues  (UNREACHABLE)
> >>>     Since the loop increment is unreachable, the loop body will never execute more than once.
> 8287       for (;;) {

Here, too.
This patch:
 (1) Passes the few unused return for these functions to mozilla::Unused, which should silence the first half of the coverity reports, and which makes it more explicit/self-documenting that these functions *are* returning a success/fail value, and we *are* intentionally ignoring it.

 (2) Marks the functions as MOZ_MUST_USE, since we now "use" the return value in 100% of the callsites. This will prompt any future callers to realize that they should probably be checking the return value (since virtually all other callers do condition on it), or to explicitly opt-out of that in a clear way via mozilla::Unused.

(Ultimately we might want to mark other nsCSSParser functions as MOZ_MUST_USE, too; but for now I'm just marking these two since those are the ones that were "implicated" in this coverity report.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8843364 - Flags: review?(xidorn+moz)
(I was going to tag dbaron for review here, but bugzilla tells me he's not accepting review requests.  I'll bet xidorn is comfortable reviewing this, though.)
Here's a green static-analysis Try run (to demonstrate that the new MOZ_MUST_USE annotations are satisfied):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed621ba316510c9bb52f1a7ea3d1836ebda4e5d0
(Also, I mentioned one more thing in comment 0's analysis of all the ParseSingleTokenVariant usages -- if ParseSingleTokenVariant fails, it puts back whatever it found. That's why it's reasonable to call it to parse "optional" things and ignore its return value.)
(sorry, s/mentioned/should've mentioned/)
I read almost every Coverity report for potential issues in layout/, but I never mind fixing things you listed here... because those are totally normal as you suggested... We should probably mark them as false-positive...
Attachment #8843364 - Flags: review?(xidorn+moz) → review+
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c8d8a074291
Annotate intentionally-unused SkipDeclaration() & ParseSingleTokenVariant() return values, and mark the functions as MOZ_MUST_USE. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/5c8d8a074291
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.