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)
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)
15.66 KB,
patch
|
dveditz
:
review+
dbaron
:
superreview+
asa
:
approval-aviary1.0.1+
asa
:
approval1.7.6+
asa
:
approval1.8b+
|
Details | Diff | Splinter Review |
14.72 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•20 years ago
|
Comment 1•19 years ago
|
||
Daniel, do you have time to work on this, by any chance?
Keywords: helpwanted
Whiteboard: oom
Reporter | ||
Comment 3•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
Is this patch ready for reviews? Would be nice to get this into the upcoming releases
Updated•19 years ago
|
Group: security
Comment 6•19 years ago
|
||
*** Bug 281701 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Attachment #173170 -
Flags: superreview?(darin)
Attachment #173170 -
Flags: review?(cbiesinger)
Updated•19 years ago
|
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
Reporter | ||
Comment 7•19 years ago
|
||
> 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.
Reporter | ||
Comment 8•19 years ago
|
||
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•19 years ago
|
||
Johnny, can you help with a super-review here?
Comment 10•19 years ago
|
||
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/
Assignee | ||
Comment 12•19 years ago
|
||
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-
Updated•19 years ago
|
Whiteboard: [sg:fix]oom --needs reviews → [sg:fix]oom --needs patch
Assignee | ||
Comment 13•19 years ago
|
||
FYI, I'm working on an improved version of this patch.
Assignee | ||
Comment 14•19 years ago
|
||
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 15•19 years ago
|
||
Comment on attachment 174104 [details] [diff] [review] v2 patch fwiw r=dveditz
Updated•19 years ago
|
Whiteboard: [sg:fix]oom --needs patch → [sg:fix]oom --needs sr dbaron
Updated•19 years ago
|
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+
Assignee | ||
Updated•19 years ago
|
Attachment #174104 -
Flags: approval1.8b?
Attachment #174104 -
Flags: approval1.7.6?
Attachment #174104 -
Flags: approval-aviary1.0.1?
Comment 17•19 years ago
|
||
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+
Assignee | ||
Comment 18•19 years ago
|
||
> 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.
Assignee | ||
Comment 19•19 years ago
|
||
fixed-on-trunk for mozilla 1.8b1 looks like most of the patch conflicts with the 1.7 and aviary branches :-/
Assignee | ||
Comment 20•19 years ago
|
||
fixed1.7.6, fixed-aviary1.0.1
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed-aviary1.0.1,
fixed1.7.6
Resolution: --- → FIXED
Whiteboard: [sg:fix]oom --needs sr dbaron → [sg:fix]oom
Assignee | ||
Comment 21•19 years ago
|
||
here's the patch that i landed on the 1.7 branch and 1.0.1 aviary branch.
Updated•19 years ago
|
Keywords: helpwanted
Updated•19 years ago
|
Group: security
Comment 22•19 years ago
|
||
can somebody with the proper bits make bug 281701 public? it is a dupe of this public bug.
Comment 23•19 years ago
|
||
darin: re: comment 14, did you ever get around to filing a bug on implementing proper error handling in the string API?
Assignee | ||
Comment 24•19 years ago
|
||
See bug 302063 (thanks for the reminder)
Updated•13 years ago
|
Crash Signature: [@nsTSubstring_CharT]
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•