Closed
Bug 1344040
Opened 7 years ago
Closed 7 years ago
Coverity defects for nsCSSParser.cpp
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Comment 2•7 years ago
|
||
(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.)
Assignee | ||
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
(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.)
Assignee | ||
Comment 5•7 years ago
|
||
(sorry, s/mentioned/should've mentioned/)
Comment 6•7 years ago
|
||
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...
Updated•7 years ago
|
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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5c8d8a074291
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•