292940 - partial OOM audit for nsStringArray and nsCStringArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

basics:
1. the string array methods (enumerate[forwards|backwords]) seem to assert that
you'd never have a null element in them, then their copy constructors (among
other creatures) proceed to (for OOM) fill them with nulls anyway.

2. some of the code willfully violates nsCRT::strtok which clearly says it does
not want null pointers:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/ds/nsCRT.cpp&rev=3.56&mark=140#138

3.  certain other functions other than the copy constructos also occasionally
decide to stash nulls into their arrays.

Comment 1

[timeless]

Attachment: handle oom

Comment 2

[Alec Flett]

Comment on attachment 184682 [details] [diff] [review]
handle oom

-	 AppendElement(new nsCString(token));
+	 nsCString *cstring = new nsCString(token);
+	 if (cstring && !AppendElement(cstring)) {
+	   delete cstring;
+	   cstring = nsnull;
+	 }
+	 if (!cstring) {
+	   RemoveElementsAt(count, Count() - count);
+	   nsCRT::free(rest);
+	   return PR_FALSE;
+	 }


Not sure the logic works here - if the new fails, and cstring is null, then the
AppendElement won't happen (short circut evaluation) So calling
RemoveElementsAt() seems wrong.

rest of the patch seems fine though

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> Not sure the logic works here - if the new fails, and cstring is null, then the
> AppendElement won't happen (short circut evaluation) So calling
> RemoveElementsAt() seems wrong.

Alec, this is fine, because what that call is trying to do is to remove all of
the other strings that *did* get added to the array. If this was the first
string that we were trying to add, then RemoveElementsAt() will simply be a no-op.

So the idea is that we want to remove all of the new elements if either the
|new| failed or appending did.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 184682 [details] [diff] [review]
handle oom

what alec said.  need a new patch.

Comment 5

[timeless]

Attachment: explain oom handling

Comment 6

[timeless]

Comment on attachment 187618 [details] [diff] [review]
explain oom handling

grumble?

Comment 7

[Darin Fisher]

Comment on attachment 187618 [details] [diff] [review]
explain oom handling

ok, r=darin

(not sure why you are requesting review from alecf... i doubt he is reviewing mozilla patches these days)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Attachment: Explain OOM handling - the un-bitrotted version

Bitrot BAAAD.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Attachment: Explain OOM handling - the un-bitrotted version that actually compiles

OK, so attempting to compile the patch before submitting it probably would have been a wise course of action. This one compiles. Really, I swear.

Comment 10

[Mike Schroepfer]

Can we get some tests added for this?

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 295592 [details] [diff] [review]
Explain OOM handling - the un-bitrotted version that actually compiles

Minusing as per schrep's comments; please add tests and renom.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

(or tell us why tests can't be written - just need to answer the question)

Comment 13

[Jesse Ruderman]

OOM tests are hard to write.  They'll either be really slow while they gobble up memory, or they'll be compiled-code tests with artificially limited memory or artificially failing malloc.  Do we have any OOM tests in the tree?

Making nsCStringArray::ParseString return a boolean (as opposed to crashing safely) without updating the callers seems kinda sketchy.  I think there are about 60 callers :/

This comment change isn't quite right:

-        /* calling AppendElement(void*) to avoid extra nsCString copy */
+        // calling AppendElement(void*) to avoid extra nsCString copy */

Comment 14

[timeless]

Comment on attachment 295592 [details] [diff] [review]
Explain OOM handling - the un-bitrotted version that actually compiles

i was hoping to fix the callers after adding the oom return
it's kinda painful to do combined. all of our string classes are broken wrt Append() - they'd silently drop any indication that they failed to Append(whatever).

Jesse isn't happy about that either...

from my perspective, blocking this while that's still as is seems relatively silly. at least we can work to get this fixed

as for oom testing, historically i did it using ulimit, the granularity is actually not good enough, so while I could find/file/fix bugs, reproducibility was a joke, because a given fix could shift the memory patterns exposing a different oom path. And anyone else anywhere along the way could cause the test to miss if their allocation pattern changed.

jesse's summary of oom test writing is fairly accurate. Generally we have two approaches for oom:
1. someone who uses a failing allocator (i manage to do this by just running out of real OS vm on various boxes, but most of the reporters we have use special libraries) and reports crashes
2. a coverity like tool (I think dreftool sometimes worked for this)

but as is, given the apis are void returns, there's no way for coverity to flag about a problem. If it's possible to hint that a given function is an allocator and needs to be checked, then we should eventually add such hints once we've made the functions indicate failure.

I don't want to be the guinea pig for oom test procedure. I have a fairly long track record of working on this area, but I do it in non existent spare time. I'm only writing this comment because reed+jesse bugged me. I'd rather be asleep in bed (I'm sick!).

Comment 15

[Mike Schroepfer]

Comment on attachment 295592 [details] [diff] [review]
Explain OOM handling - the un-bitrotted version that actually compiles

Can you make sure to get a bug on file to clean up callers and make it block this one?

Comment 16

[timeless]

Comment on attachment 295592 [details] [diff] [review]
Explain OOM handling - the un-bitrotted version that actually compiles

mozilla/xpcom/glue/nsVoidArray.cpp 	3.49
mozilla/xpcom/glue/nsVoidArray.h 	3.39

filed bug 412343 for the followup work