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

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 beta2+, blocking1.9.2 .7+, status1.9.2 .7-fixed, blocking1.9.1 .11+, status1.9.1 .11-fixed)

Details

(Whiteboard: [sg:critical?], URL)

Attachments

(2 attachments)

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
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(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]
fix

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:
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.
Status: ASSIGNED → RESOLVED
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Last Resolved: 7 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?]
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).
Attachment #454109 - Flags: approval1.9.2.6?
Attachment #454109 - Flags: approval1.9.1.11?
Attachment #454105 - Attachment description: fix? → fix

Updated

7 years ago
blocking1.9.1: ? → .11+
blocking1.9.2: ? → .6+
status1.9.1: --- → wanted
status1.9.2: --- → wanted

Comment 7

7 years ago
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!
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.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f2a4c8e82d16
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/025191a13eb7
status1.9.1: wanted → .11-fixed
status1.9.2: wanted → .6-fixed
blocking2.0: ? → beta2+
Is there a means for QA to verify this fix for 1.9.2.7 and 1.9.1.11?
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.