Last Comment Bug 737164 - Make strings malloc-infallible by default
: Make strings malloc-infallible by default
Status: RESOLVED FIXED
: sec-want
Product: Core
Classification: Components
Component: String (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Benjamin Smedberg [:bsmedberg]
:
: Nathan Froyd [:froydnj]
Mentors:
: 383570 (view as bug list)
Depends on: 736501 755323 767343 770262 804742
Blocks: 427099 742614 1027163
  Show dependency treegraph
 
Reported: 2012-03-19 13:28 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2014-06-20 06:21 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Part A - nsTSubstring and XPCOM, rev. 1 (25.29 KB, patch)
2012-03-19 13:32 PDT, Benjamin Smedberg [:bsmedberg]
justin.lebar+bug: review+
dbaron: feedback+
Details | Diff | Splinter Review
Part B - necko changes, checkpoint (2.76 KB, text/plain)
2012-03-19 13:32 PDT, Benjamin Smedberg [:bsmedberg]
jduell.mcbugs: review+
Details
Part C - "Other" changes, checkpoint (3.85 KB, text/plain)
2012-03-19 13:33 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details
Move the declaration of fallible_t to its own header, rev. 1 (2.54 KB, patch)
2012-05-01 11:13 PDT, Benjamin Smedberg [:bsmedberg]
cjones.bugs: review+
Details | Diff | Splinter Review
Move the declaration of fallible_t to its own header, rev. 1.1 (2.57 KB, patch)
2012-05-01 11:19 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Splinter Review
Make ReadInputStreamToString fallible (1.00 KB, patch)
2012-05-11 01:38 PDT, Jason Duell [:jduell] (needinfo me)
benjamin: review+
Details | Diff | Splinter Review
Make ReadInputStreamToString fallible, v2 (1.46 KB, patch)
2012-05-11 16:26 PDT, Jason Duell [:jduell] (needinfo me)
benjamin: review+
Details | Diff | Splinter Review

Description Benjamin Smedberg [:bsmedberg] 2012-03-19 13:28:05 PDT
Our current string API is supposedly malloc-safe, but in practice this means that callers don't check return values and so many callers are not actually OOM-safe. In this bug I intend to make the default version of all API calls infallible (abort on OOM) and provide an explicitly fallible version the same way we do for "operator new":

bool NS_WARN_UNUSED_RESULT Assign(const nsSubstring_type& other, const mozilla::fallible_t&);
Comment 1 Benjamin Smedberg [:bsmedberg] 2012-03-19 13:32:04 PDT
Created attachment 607282 [details] [diff] [review]
Part A - nsTSubstring and XPCOM, rev. 1

I will do the subclasses (nsTString etc) as a separate patch for ease of commit and review. This could be committed as-is with the minor tree fixups in part B/C
Comment 2 Benjamin Smedberg [:bsmedberg] 2012-03-19 13:32:42 PDT
Created attachment 607284 [details]
Part B - necko changes, checkpoint
Comment 3 Benjamin Smedberg [:bsmedberg] 2012-03-19 13:33:06 PDT
Created attachment 607285 [details]
Part C - "Other" changes, checkpoint
Comment 4 Justin Lebar (not reading bugmail) 2012-03-20 09:00:44 PDT
> bool NS_WARN_UNUSED_RESULT Assign(const nsSubstring_type& other, const mozilla::fallible_t&);

What's the advantage of this as opposed to

> bool NS_WARN_UNUSED_RESULT FallibleAssign(const nsSubstring_type& other)

?  I guess it's nice that fallible_t() matches our fallible operator new, but perhaps new should be the exception, rather than the rule?
Comment 5 Justin Lebar (not reading bugmail) 2012-03-20 09:10:25 PDT
Why aren't there infallible versions of EnsureMutable and ReplacePrep?  Seems like that would save a bunch of if statements.
Comment 6 Benjamin Smedberg [:bsmedberg] 2012-03-20 09:18:13 PDT
I don't think that having alternately-named methods would be more readable: it's nice (to me at least!) to have fallible_t as the trigger for reviewers and coders that a method needs extra attention.

EnsureMutable and ReplacePrep were both internal (protected) methods, so I was mainly doing the expedient thing with them. If you think it would generate faster code to make an infallible version and remove some of the checks, I can try it: I'd need to look at the generated code and make sure that passing fallible_t doesn't actually run extra code in optimized builds; I'd expect that the caller would have to allocate stack space for the parameter but that neither side actually needs to touch that stack space. I'd be very impressed and happy if MSVC LTCG could remove the parameter altogether.
Comment 7 Justin Lebar (not reading bugmail) 2012-03-20 09:32:11 PDT
> I don't think that having alternately-named methods would be more readable: it's nice (to me at 
> least!) to have fallible_t as the trigger for reviewers and coders that a method needs extra 
> attention. 

It's not a big deal either way.

> If you think it would generate faster code to make an infallible version and remove some of the 
> checks, I can try it

I suppose it's possible that code would run faster if you pushed down the if statement into EnsureMutable / ReplacePrep -- it's less work for the branch predictor, assuming they don't get inlined.  But I doubt this matters; I was mainly speaking from a cleanliness POV -- the fewer places in the code where you handle OOM, the better.

It probably wouldn't be a bad idea to annotate the OOM if statement as unlikely, though.

> I'd be very impressed and happy if MSVC LTCG could remove the parameter altogether.

Hopefully most of the calls you care about being fast are inlined anyway.
Comment 8 Justin Lebar (not reading bugmail) 2012-03-20 12:04:09 PDT
> +    // There are not fallible version of these methods because they only really
> +    // apply to small allocations that we wouldn't want to check anyway.

Nit: s/really//

My preference would still be to push the aborts down into EnsureMutable / ReplacePrep, unless there's a good reason not to, so things like the unlikely branch annotation are easier to write.  But r=me either way.
Comment 9 Benjamin Smedberg [:bsmedberg] 2012-05-01 11:13:28 PDT
Created attachment 619991 [details] [diff] [review]
Move the declaration of fallible_t to its own header, rev. 1
Comment 10 Benjamin Smedberg [:bsmedberg] 2012-05-01 11:19:27 PDT
Created attachment 619998 [details] [diff] [review]
Move the declaration of fallible_t to its own header, rev. 1.1

Bah, need C++ include guard.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-01 23:21:32 PDT
Comment on attachment 619998 [details] [diff] [review]
Move the declaration of fallible_t to its own header, rev. 1.1

I'm not sure which patch of these patches I was supposed to review, but ...
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-01 23:22:33 PDT
Comment on attachment 619991 [details] [diff] [review]
Move the declaration of fallible_t to its own header, rev. 1

... I prefer leaving the #include at the top of the file.  Please put it in the #if defined(__cplusplug) just above here.

r=me with that
Comment 13 Jason Duell [:jduell] (needinfo me) 2012-05-08 11:23:31 PDT
Comment on attachment 607284 [details]
Part B - necko changes, checkpoint

I think we want to keep NS_ReadInputStreamToString fallible, at least for now.  Websockets uses it to slurp up arbitrarily large blobs:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#359

Other places where we use the function to read in whole (but prob small enough) files: mozJSSubScriptLoader::ReadScript(), nsFrameScriptExecutor::LoadFrameScriptInternal().
Comment 14 Jason Duell [:jduell] (needinfo me) 2012-05-08 11:48:48 PDT
Websockets will no longer slurp whole blob in once bug 704447 is fixed, BTW.
Comment 16 Joe Drew (not getting mail) 2012-05-10 18:34:37 PDT
https://hg.mozilla.org/mozilla-central/rev/45ffd4b20e11
Comment 17 Jason Duell [:jduell] (needinfo me) 2012-05-11 01:31:38 PDT
We landed the necko patch and closed this w/o addressing comment 13. Everything that's checked in is fine, but I do think we also need to make the ReadInputStreamToString change.
Comment 18 Jason Duell [:jduell] (needinfo me) 2012-05-11 01:38:40 PDT
Created attachment 623066 [details] [diff] [review]
Make ReadInputStreamToString fallible

and here's the patch...
Comment 19 Benjamin Smedberg [:bsmedberg] 2012-05-11 07:53:27 PDT
Comment on attachment 623066 [details] [diff] [review]
Make ReadInputStreamToString fallible

sorry, I somehow got confused on which patches were reviewed :-(
Comment 20 Jason Duell [:jduell] (needinfo me) 2012-05-11 14:17:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a88c34de5d01
Comment 21 Jason Duell [:jduell] (needinfo me) 2012-05-11 16:25:18 PDT
So my patch burned up the inbound tree.  The issue is that nsNetUtil.h is included and used by toolkit/system/gnome/nsAlertsIconListener.cpp which doesn't define MOZILLA_INTERNAL_API.

This patch fixes by wrapping just the NS_ReadInputStreamToString function in #ifdef MOZILLA_INTERNAL_API.  Alternately we could have sAlertsIconListener.cpp stop using NS_NewURI.

We're not changing the external string API to be infallible, are we?
Comment 22 Jason Duell [:jduell] (needinfo me) 2012-05-11 16:26:36 PDT
Created attachment 623357 [details] [diff] [review]
Make ReadInputStreamToString fallible, v2
Comment 23 Benjamin Smedberg [:bsmedberg] 2012-05-12 08:38:37 PDT
Comment on attachment 623357 [details] [diff] [review]
Make ReadInputStreamToString fallible, v2

The external API has become infallible because it basically forwards to the internal API.
Comment 24 Jason Duell [:jduell] (needinfo me) 2012-05-14 14:36:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdd1e7c33358
Comment 25 Jason Duell [:jduell] (needinfo me) 2012-05-14 17:31:35 PDT
Comment on attachment 607284 [details]
Part B - necko changes, checkpoint

belated +r to already-landed patch
Comment 26 Benjamin Smedberg [:bsmedberg] 2012-05-14 17:58:14 PDT
Requesting tracking because this has the possibility of causing new crashes (with mozalloc_abort | * signatures) if there are places which currently may have content-controlled large allocations and do have string API checks already in place.
Comment 27 Ed Morley [:emorley] 2012-05-15 06:35:53 PDT
https://hg.mozilla.org/mozilla-central/rev/bdd1e7c33358
Comment 28 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-16 12:06:08 PDT
marking for tracking as per comment 26, if there are regressions please update the status flag so this gets on our radar again.
Comment 29 Jesse Ruderman 2013-02-20 10:09:02 PST
*** Bug 383570 has been marked as a duplicate of this bug. ***

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