Last Comment Bug 659963 - Consistently use infallible memory allocation in CSS parser
: Consistently use infallible memory allocation in CSS parser
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla10
Assigned To: Zack Weinberg (:zwol)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 543151
Blocks: 516091 907547
  Show dependency treegraph
 
Reported: 2011-05-26 09:03 PDT by Zack Weinberg (:zwol)
Modified: 2013-08-20 20:42 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (23.82 KB, patch)
2011-05-26 09:03 PDT, Zack Weinberg (:zwol)
dbaron: review+
Details | Diff | Splinter Review
v2 (43.48 KB, patch)
2011-05-26 14:43 PDT, Zack Weinberg (:zwol)
dbaron: review+
Details | Diff | Splinter Review
v2 updated and checked in (43.53 KB, patch)
2011-10-19 18:56 PDT, Zack Weinberg (:zwol)
zackw: review+
zackw: checkin+
Details | Diff | Splinter Review

Description Zack Weinberg (:zwol) 2011-05-26 09:03:55 PDT
Created attachment 535357 [details] [diff] [review]
patch

Some, but by no means all, memory allocation points in the CSS parser try to recover from allocation failure by setting a "low level error" flag to NS_ERROR_OUT_OF_MEMORY and then returning PR_FALSE to higher layers.  Higher layers invariably treat a PR_FALSE return from a parser subroutine as indicating a *syntax* error, so the behavior on allocation failure is likely to be to attempt syntax error recovery, inappropriately, and then barge onwards.  The very highest parser functions do check the "low level error" and report to their callers, but nearly every *caller* of those functions ignores error returns.  So, in summary, it's a big mess.  I propose to rip the whole mess out and just rely on infallible operator new, infallible nsTArray resize, etc. throughout nsCSSParser.

This was originally part of bug 543151 and probably has a textual dependency on the current patch series in that bug.  I recall folks expressing skepticism about it (but not the details - sorry, it was some time ago) so I'm pulling it out to its own bug.

This is not an exhaustive audit of memory allocation points in layout/style, it only covers allocations that could cause a call to nsCSSScanner::SetLowLevelError.
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-05-26 09:07:23 PDT
In general, when web content can control the allocation sizes, we do not want to use the infallible allocator. This seems like one of the obvious cases (apart from the code perhaps being incorrect currently) where we *do* want a fallible allocator.
Comment 2 Zack Weinberg (:zwol) 2011-05-26 09:15:58 PDT
Consistent use of fallible allocation is not feasible with the code as it stands: every call site in the parser would have to be changed to detect and propagate memory allocation failures.  Inconsistent behavior upon allocation failure is, IMO, worse than consistent use of infallible allocation, even for allocations directly caused by content.

Also, the amount of memory allocated for the parse of a style sheet is at most linear in the size of the sheet itself; this is not like images or script execution, where a small file can cause arbitrarily large allocations.  (It is probably comparable to script *parsing* - I don't know how that works.)
Comment 3 David Baron :dbaron: ⌚️UTC-10 2011-05-26 12:17:36 PDT
Comment on attachment 535357 [details] [diff] [review]
patch

In the else-if chain in ParseColorString, please change the else-ifs
to just ifs, and make the first if return NS_ERROR_FAILURE inside the
outer if but outside the inner if.

Could you make nsMediaQuery::mExpressions be explicitly
InfallibleTArray<> rather than nsTArray<> (even though they're now
the same).

Same for nsMediaList::mArray.

Please change the return type of CSSParserImpl::PushGroup to return
void, and likewise change mGroupStack to InfallibleTArray.

In ParsePseudoSelector, do_GetAtom is currently fallible.  So please
add NS_RUNTIMEABORT there as well.

Likewise, make nsCSSValueGradient::mStops InfallibleTArray.

Make CSSParserImpl::ParseFunctionInternals take InfallibleTArray,
and ParseFunction use it.

r=dbaron with that
Comment 4 Zack Weinberg (:zwol) 2011-05-26 14:43:34 PDT
Created attachment 535481 [details] [diff] [review]
v2

(In reply to comment #3)
> In the else-if chain in ParseColorString, please change the else-ifs
> to just ifs, and make the first if return NS_ERROR_FAILURE inside the
> outer if but outside the inner if.

Done.

> Could you make nsMediaQuery::mExpressions be explicitly
> InfallibleTArray<> rather than nsTArray<> (even though they're now
> the same).
[etc]

After rereading the code I changed all uses of nsTArray<> in the parser to InfallibleTArray<>.  There were several places that were already assuming infallible resize semantics.

> Please change the return type of CSSParserImpl::PushGroup to return
> void, and likewise change mGroupStack to InfallibleTArray.

Done.  Sorry about that, I meant to do it but forgot.

> In ParsePseudoSelector, do_GetAtom is currently fallible.  So please
> add NS_RUNTIMEABORT there as well.

Done (also several other places where do_GetAtom was being called without any check).

Given slightly more changes than you asked for, do you want to re-review?
Comment 5 David Baron :dbaron: ⌚️UTC-10 2011-05-26 16:14:07 PDT
Comment on attachment 535481 [details] [diff] [review]
v2

r=dbaron
Comment 6 Zack Weinberg (:zwol) 2011-10-19 18:56:46 PDT
Created attachment 568274 [details] [diff] [review]
v2 updated and checked in

Dusted this off (some skew, but no significant changes) and pushed to -inbound with the first two pieces of bug 543151.

https://hg.mozilla.org/integration/mozilla-inbound/rev/67673422f7d2
Comment 7 Marco Bonardo [::mak] 2011-10-20 03:15:32 PDT
https://hg.mozilla.org/mozilla-central/rev/67673422f7d2

Note You need to log in before you can comment on or make changes to this bug.