Closed Bug 574750 Opened 10 years ago Closed 10 years ago

Code that iterates through nsCSSValue::Arrays should use size_t for index now


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
blocking2.0 --- beta2+
blocking1.9.2 --- .7+
status1.9.2 --- .7-fixed
blocking1.9.1 --- .11+
status1.9.1 --- .11-fixed


(Reporter: dholbert, Assigned: dholbert)




(Whiteboard: [sg:critical?])


(2 files)

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.

278     nsCSSValue::Array *array = aValue.GetArrayValue();
330     for (PRUint16 index = 1; index < array->Count(); ++index) {
331       AppendCSSValueToString(aProperty, array->Item(index), aResult);
333       /* If we're not at the final element, append a comma. */
334       if (index + 1 != array->Count())
335         aResult.AppendLiteral(", ");
336     }
Attached patch fixSplinter Review
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.
Assignee: nobody → dholbert
(meant to say: haven't compiled with the attached patch yet -- doing that now)
OS: Linux → All
Hardware: x86 → All
Comment on attachment 454105 [details] [diff] [review]

This builds fine on my machine.  bz, would you mind reviewing this?
Attachment #454105 - Flags: review?(bzbarsky)
Attachment #454105 - Flags: review?(bzbarsky) → review+
Landed on trunk:

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.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Closed: 10 years ago
Resolution: --- → FIXED
Marking as security-sensitive at reed's suggestion, to be on the safe side.
Group: core-security
blocking2.0: --- → ?
Whiteboard: [sg:critical?]
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).
Attachment #454109 - Flags: approval1.9.2.6?
Attachment #454109 - Flags: approval1.9.1.11?
Attachment #454105 - Attachment description: fix? → fix
blocking1.9.1: ? → .11+
blocking1.9.2: ? → .6+
Comment on attachment 454109 [details] [diff] [review]
fix for 1.9.2 & 1.9.1

a=LegNeato for and

Please submit to mozilla-1.9.2 default and mozilla-1.9.1 default asap, as code freeze is tonight!
Attachment #454109 - Flags: approval1.9.2.6?
Attachment #454109 - Flags: approval1.9.2.6+
Attachment #454109 - Flags: approval1.9.1.11?
Attachment #454109 - Flags: approval1.9.1.11+
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.
Is there a means for QA to verify this fix for and
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).
Group: core-security
You need to log in before you can comment on or make changes to this bug.