Last Comment Bug 277549 - Out of memory in MutatePrep is not well handled [@nsTSubstring_CharT]
: Out of memory in MutatePrep is not well handled [@nsTSubstring_CharT]
Status: RESOLVED FIXED
[sg:fix]oom
: crash, fixed-aviary1.0.1, fixed1.7.6
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Darin Fisher
:
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
: 281701 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-01-08 09:57 PST by Daniel de Wildt
Modified: 2006-03-12 18:15 PST (History)
11 users (show)
dveditz: blocking1.7.6+
dveditz: blocking‑aviary1.0.1+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposal how to handle oom in MutatePrep (9.43 KB, patch)
2005-02-02 06:03 PST, Daniel de Wildt
darin.moz: review-
Details | Diff | Review
v2 patch (15.66 KB, patch)
2005-02-11 15:36 PST, Darin Fisher
dveditz: review+
dbaron: superreview+
asa: approval‑aviary1.0.1+
asa: approval1.7.6+
asa: approval1.8b+
Details | Diff | Review
patch for the 1.7 based branches (14.72 KB, patch)
2005-02-15 23:34 PST, Darin Fisher
no flags Details | Diff | Review

Description Daniel de Wildt 2005-01-08 09:57:47 PST
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.
Comment 1 Boris Zbarsky [:bz] 2005-01-30 21:31:57 PST
Daniel, do you have time to work on this, by any chance?
Comment 2 Daniel de Wildt 2005-02-02 05:44:19 PST
I will try it.
Comment 3 Daniel de Wildt 2005-02-02 06:03:55 PST
Created attachment 173170 [details] [diff] [review]
Proposal how to handle oom in MutatePrep

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?
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2005-02-06 09:35:59 PST
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?
Comment 5 Daniel Veditz [:dveditz] 2005-02-09 16:15:18 PST
Is this patch ready for reviews? Would be nice to get this into the upcoming
releases
Comment 6 Daniel Veditz [:dveditz] 2005-02-09 20:41:37 PST
*** Bug 281701 has been marked as a duplicate of this bug. ***
Comment 7 Daniel de Wildt 2005-02-10 03:15:41 PST
> 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.
Comment 8 Daniel de Wildt 2005-02-10 07:54:43 PST
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?
Comment 9 Asa Dotzler [:asa] 2005-02-10 12:49:10 PST
Johnny, can you help with a super-review here?
Comment 10 Christian :Biesinger (don't email me, ping me on IRC) 2005-02-10 13:32:09 PST
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
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-02-10 13:35:16 PST
Comment on attachment 173170 [details] [diff] [review]
Proposal how to handle oom in MutatePrep

>+         * returns PR_FALSE of reallocation fails

s/of/if/
Comment 12 Darin Fisher 2005-02-11 09:13:19 PST
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.
Comment 13 Darin Fisher 2005-02-11 14:00:06 PST
FYI, I'm working on an improved version of this patch.
Comment 14 Darin Fisher 2005-02-11 15:36:29 PST
Created attachment 174104 [details] [diff] [review]
v2 patch

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.
Comment 15 Daniel Veditz [:dveditz] 2005-02-11 16:47:22 PST
Comment on attachment 174104 [details] [diff] [review]
v2 patch

fwiw r=dveditz
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-02-15 16:09:36 PST
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
Comment 17 Asa Dotzler [:asa] 2005-02-15 22:16:37 PST
Comment on attachment 174104 [details] [diff] [review]
v2 patch

a=asa for checkin everwhere.
Comment 18 Darin Fisher 2005-02-15 23:19:28 PST
> 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.
Comment 19 Darin Fisher 2005-02-15 23:21:59 PST
fixed-on-trunk for mozilla 1.8b1

looks like most of the patch conflicts with the 1.7 and aviary branches :-/
Comment 20 Darin Fisher 2005-02-15 23:33:06 PST
fixed1.7.6, fixed-aviary1.0.1
Comment 21 Darin Fisher 2005-02-15 23:34:04 PST
Created attachment 174457 [details] [diff] [review]
patch for the 1.7 based branches

here's the patch that i landed on the 1.7 branch and 1.0.1 aviary branch.
Comment 22 Marc Bejarano 2005-02-26 13:47:59 PST
can somebody with the proper bits make bug 281701 public?  it is a dupe of this
public bug.
Comment 23 Marc Bejarano 2005-05-28 10:23:13 PDT
darin: re: comment 14, did you ever get around to filing a bug on implementing
proper error handling in the string API?
Comment 24 Darin Fisher 2005-07-25 11:19:52 PDT
See bug 302063 (thanks for the reminder)

Note You need to log in before you can comment on or make changes to this bug.