Last Comment Bug 574750 - Code that iterates through nsCSSValue::Arrays should use size_t for index now
: Code that iterates through nsCSSValue::Arrays should use size_t for index now
Status: RESOLVED FIXED
[sg:critical?]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
http://tinderbox.mozilla.org/showlog....
Depends on: CVE-2010-2752
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-25 11:50 PDT by Daniel Holbert [:dholbert]
Modified: 2010-09-27 18:02 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta2+
.7+
.7-fixed
.11+
.11-fixed


Attachments
fix (6.13 KB, patch)
2010-06-25 12:04 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review
fix for 1.9.2 & 1.9.1 (3.57 KB, patch)
2010-06-25 12:58 PDT, Daniel Holbert [:dholbert]
christian: approval1.9.2.7+
christian: approval1.9.1.11+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2010-06-25 11:50:01 PDT
Now that Bug 574059 landed and nsCSSValue::Array::Count() returns a size_t (instead of PRUint16), we need to do a s/PRUint16/size_t/ on code that iterates through these arrays, too.

e.g.
278     nsCSSValue::Array *array = aValue.GetArrayValue();
[...]
330     for (PRUint16 index = 1; index < array->Count(); ++index) {
331       AppendCSSValueToString(aProperty, array->Item(index), aResult);
332 
333       /* If we're not at the final element, append a comma. */
334       if (index + 1 != array->Count())
335         aResult.AppendLiteral(", ");
336     }
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSDeclaration.cpp
Comment 1 Daniel Holbert [:dholbert] 2010-06-25 12:04:21 PDT
Created attachment 454105 [details] [diff] [review]
fix

I think this catches all the instances.  There are a few that don't matter as much (e.g. in nsStyleAnimation, where we use a PRUint32 as the loop counter, and we're only iterating up to 4), but I figured we might as well fix those too for consistency.
Comment 2 Daniel Holbert [:dholbert] 2010-06-25 12:05:22 PDT
(meant to say: haven't compiled with the attached patch yet -- doing that now)
Comment 3 Daniel Holbert [:dholbert] 2010-06-25 12:20:28 PDT
Comment on attachment 454105 [details] [diff] [review]
fix

This builds fine on my machine.  bz, would you mind reviewing this?
Comment 4 Daniel Holbert [:dholbert] 2010-06-25 12:42:24 PDT
Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/619563f026f5

Requesting blocking1.9.1 & 1.9.2, to avoid complications on those branches from comparing PRUint16 list-counter vs. a size_t Count() method as a result of bug 574059.
Comment 5 Daniel Holbert [:dholbert] 2010-06-25 12:51:35 PDT
Marking as security-sensitive at reed's suggestion, to be on the safe side.
Comment 6 Daniel Holbert [:dholbert] 2010-06-25 12:58:33 PDT
Created attachment 454109 [details] [diff] [review]
fix for 1.9.2 & 1.9.1

Here's the fix backported to 1.9.2 -- basically just removes the chunks for CSS calc() and nsStyleAnimation (which are new on m-c since 1.9.2 branched), and fixes contextual code.

This patch applies fine to 1.9.1, too, with fuzz=3 (for minor changes in contextual code).
Comment 7 christian 2010-06-25 13:01:48 PDT
Comment on attachment 454109 [details] [diff] [review]
fix for 1.9.2 & 1.9.1

a=LegNeato for 1.9.2.6 and 1.9.2.11.

Please submit to mozilla-1.9.2 default and mozilla-1.9.1 default asap, as code freeze is tonight!
Comment 8 Daniel Holbert [:dholbert] 2010-06-25 13:03:23 PDT
Thanks for the quick turnaround on approval!

I'm going to wait for this to cycle on mozilla-central first, before landing the 1.9.1 / 1.9.2 version.
Comment 10 Al Billings [:abillings] 2010-07-16 16:11:24 PDT
Is there a means for QA to verify this fix for 1.9.2.7 and 1.9.1.11?
Comment 11 Daniel Holbert [:dholbert] 2010-07-16 16:19:07 PDT
Only via code inspection (making sure the patch landed).

I noticed the problem via code inspection in the first place -- we never had a testcase (though it's probably possible to make one).

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