Closed Bug 277549 Opened 20 years ago Closed 19 years ago

Out of memory in MutatePrep is not well handled [@nsTSubstring_CharT]

Categories

(Core :: XPCOM, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: dewildt, Assigned: darin.moz)

References

()

Details

(Keywords: crash, fixed-aviary1.0.1, fixed1.7.6, Whiteboard: [sg:fix]oom)

Crash Data

Attachments

(2 files, 1 obsolete file)

MutatePrep fails if the system is out of memory. This is not handled by
ReplacePrep and SetCapacity. Memory is overwritten because ReplacePrep is used
in string manipulating routines like Copy and Assign.
Daniel, do you have time to work on this, by any chance?
Keywords: helpwanted
Whiteboard: oom
I will try it.
Assignee: string → mozilla3q04
MutatePrep is used in a) ReplacePrep and b) Setcapacity. In case of
a) If MutatePrep fails, then also ReplacePrep should fail.
   The result of ReplacePrep should be handled before performing 
   the string operation.
   => In oom situations are some string operations not performed
      without any information or warning to the caller. (new bug?)

b) I'm not sure how to handle oom here.
   1) Should SetCapacity return a result which is handled later?
      (Like it is done in the patch for SetLength)
      I have no overview how far this effects other (base or derived) 
      classes.
   2) Give only an assertion?
you can't assert that an allocation succeeds. assertions are for programming errors.


As for handling OOM in general... having every function return an error code is
maybe not the best idea... maybe something like a member variable indicating
whether OOM occured and a function to to query it (this would allow doing
various stuff with a string and only checking for OOM at the end). but that also
kinda sucks...

other ideas?
Is this patch ready for reviews? Would be nice to get this into the upcoming
releases
Blocks: 218701
Flags: blocking1.7.6?
Flags: blocking-aviary1.0.1?
Blocks: 281701
No longer blocks: 218701
Group: security
No longer blocks: 281701
*** Bug 281701 has been marked as a duplicate of this bug. ***
Attachment #173170 - Flags: superreview?(darin)
Attachment #173170 - Flags: review?(cbiesinger)
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1?
Flags: blocking-aviary1.0.1+
Whiteboard: oom → [sg:fix]oom --needs reviews
> Is this patch ready for reviews? 

The changes for SetCapacity does not match the base class. (But VC give no
compile errors.) The other functions are ready. Only OOM checks are included, no
further error handling as mentioned in comment 4 is done.

If it's ok and you have a few (10) hours, then I could create a new reviewable
patch without changing the SetCapacity return type and without further error
handling. Otherwise someone else should take the bug.
The problem is SetLength, which sets the length of the buffer.

1) I think the best way to handle oom in SetCapacity is by setting a new member
variable (as suggested by comment). The variable should reflect the result of
MutatePrep and could be used outside SetCapacity and ReplacePrep (e.g. SetLength).

or

2) Change SetCapacity as proposed in the attachment, which also includes a
change of SetCapacity in nsTAString_CharT. I don't think this a good idea.

Other recommendations?
Johnny, can you help with a super-review here?
Comment on attachment 173170 [details] [diff] [review]
Proposal how to handle oom in MutatePrep

I really don't think I'm the best person to review string changes. changing to
r?darin sr?dbaron
Attachment #173170 - Flags: superreview?(dbaron)
Attachment #173170 - Flags: superreview?(darin)
Attachment #173170 - Flags: review?(darin)
Attachment #173170 - Flags: review?(cbiesinger)
Comment on attachment 173170 [details] [diff] [review]
Proposal how to handle oom in MutatePrep

>+         * returns PR_FALSE of reallocation fails

s/of/if/
Comment on attachment 173170 [details] [diff] [review]
Proposal how to handle oom in MutatePrep

Please avoid changing the public string API.  If you need to return a value
from SetCapacity so that you can check it in SetLength, then I think we should
move the body of SetCapacity into a private method, and call it from both
places.

While in the long run, I think it makes sense to add some sort of error
reporting to the public string API, I think that would involve more than just
adding a return code to SetCapacity.  Anyways, SetCapacity need not succeed if
you consider that it is only an advisory call.	If anything SetLength should
report errors, but again let's save API changes for later and attack the entire
API at that point.
Attachment #173170 - Flags: superreview?(dbaron)
Attachment #173170 - Flags: review?(darin)
Attachment #173170 - Flags: review-
Whiteboard: [sg:fix]oom --needs reviews → [sg:fix]oom --needs patch
FYI, I'm working on an improved version of this patch.
Attached patch v2 patchSplinter Review
This patch is similar to the original with the following changes:

* In SetLength(), I use Capacity() to verify that SetCapacity() succeeded.  I
know this is less efficient than a return code, but I doubt the difference is
significant.  At some point this will go away when we have proper error
handling in the API itself.

* In MutatePrep, I got rid of the code that changes mData to point to the fixed
buffer in case realloc fails because it seemed pointless given that when
realloc fails it does not modify the original buffer.  So, it is possible for
MutatePrep to instead return without modifying any state.

I'll file a follow-up bug on implementing proper error handling in the string
API unless such a bug doesn't already exist.
Assignee: mozilla3q04 → darin
Attachment #173170 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #174104 - Flags: superreview?(dbaron)
Attachment #174104 - Flags: review?(dbaron)
Comment on attachment 174104 [details] [diff] [review]
v2 patch

fwiw r=dveditz
Whiteboard: [sg:fix]oom --needs patch → [sg:fix]oom --needs sr dbaron
Attachment #174104 - Flags: review?(dbaron) → review+
Comment on attachment 174104 [details] [diff] [review]
v2 patch

>-// XXXdarin what is wrong with STDC's tolower?
>+// XXX(darin): what is wrong with STDC's tolower?

Probably that it's allowed to be locale-sensitive, although I couldn't get it
to do weird things with 'i's in tr_TR.

sr=dbaron
Attachment #174104 - Flags: superreview?(dbaron) → superreview+
Attachment #174104 - Flags: approval1.8b?
Attachment #174104 - Flags: approval1.7.6?
Attachment #174104 - Flags: approval-aviary1.0.1?
Comment on attachment 174104 [details] [diff] [review]
v2 patch

a=asa for checkin everwhere.
Attachment #174104 - Flags: approval1.8b?
Attachment #174104 - Flags: approval1.8b+
Attachment #174104 - Flags: approval1.7.6?
Attachment #174104 - Flags: approval1.7.6+
Attachment #174104 - Flags: approval-aviary1.0.1?
Attachment #174104 - Flags: approval-aviary1.0.1+
> Probably that it's allowed to be locale-sensitive, although I couldn't get it
> to do weird things with 'i's in tr_TR.

OK, i changed the comment accordingly.
fixed-on-trunk for mozilla 1.8b1

looks like most of the patch conflicts with the 1.7 and aviary branches :-/
fixed1.7.6, fixed-aviary1.0.1
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [sg:fix]oom --needs sr dbaron → [sg:fix]oom
here's the patch that i landed on the 1.7 branch and 1.0.1 aviary branch.
Keywords: helpwanted
Group: security
can somebody with the proper bits make bug 281701 public?  it is a dupe of this
public bug.
darin: re: comment 14, did you ever get around to filing a bug on implementing
proper error handling in the string API?
See bug 302063 (thanks for the reminder)
Crash Signature: [@nsTSubstring_CharT]
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.