Closed Bug 793253 Opened 7 years ago Closed 7 years ago

Supposedly infallible TArray can fail to allocate a buffer


(Core :: XPCOM, defect)

Not set



Tracking Status
firefox17 + fixed
firefox18 + fixed
firefox-esr10 --- wontfix
firefox-esr17 --- fixed


(Reporter: bzbarsky, Assigned: bzbarsky)


(Keywords: sec-moderate, Whiteboard: [adv-track-main17+][qa?])


(1 file)

nsTArray_base<Alloc>::EnsureCapacity has this bit:

  if ((uint64_t)capacity * elemSize > size_type(-1)/2) {
    NS_ERROR("Attempting to allocate excessively large array");
    return false;

If Alloc is nsTArrayInfallibleAllocator, we should be aborting here, instead of returning false, I believe.
I'm not convinced about the checkin comment if we want to not make this obvious... suggestions?
Attachment #663499 - Flags: review?
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Attachment #663499 - Flags: review? → review?(justin.lebar+bug)
Comment on attachment 663499 [details] [diff] [review]
Infallible TArrays should really be infallible.

I'm not convinced anyone would have an easy time developing a testcase out of this, and anyway the patch speaks for itself, so I don't feel like we need to obfuscate the commit message, personally.
Attachment #663499 - Flags: review?(justin.lebar+bug) → review+
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla18
Closed: 7 years ago
Resolution: --- → FIXED
We should probably get this into Firefox 17, but unless we've found a specific case where this gets abused we don't need to over-worry about esr10 and rushing into Firefox 16 at the last minute.
Let's get this into 17 so it's in the first ESR17 and also nominate for esr10.
sorry, cancel that esr10 request, i see now that we marked wontfix there.
Comment on attachment 663499 [details] [diff] [review]
Infallible TArrays should really be infallible.

OK, requesting approval for 17...

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long-standing bug
User impact if declined: Probably none, but if there is impact it would be a
   security bug.
Testing completed (on m-c, etc.): On trunk for a while, and seems OK.
Risk to taking this patch (and alternatives if risky): Main risk is if a caller
   was depending on this non-fatal behavior in an OOM situation.  Then this
   fix would introduce a DoS attack.
String or UUID changes made by this patch: None.
Attachment #663499 - Flags: approval-mozilla-beta?
Comment on attachment 663499 [details] [diff] [review]
Infallible TArrays should really be infallible.

Approving for uplift so we get this not only for Beta but for our first ESR17 builds.
Attachment #663499 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-track-main17+]
Is there something QA can do to verify this fix?
Whiteboard: [adv-track-main17+] → [adv-track-main17+][qa?]
Group: core-security
You need to log in before you can comment on or make changes to this bug.