Make strings malloc-infallible by default

RESOLVED FIXED in Firefox 15

Status

()

Core
String
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

(Blocks: 1 bug, {sec-want})

unspecified
mozilla15
x86_64
Linux
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15+ fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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&);
(Assignee)

Comment 1

6 years ago
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
Attachment #607282 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 2

6 years ago
Created attachment 607284 [details]
Part B - necko changes, checkpoint
(Assignee)

Comment 3

6 years ago
Created attachment 607285 [details]
Part C - "Other" changes, checkpoint
(Assignee)

Updated

6 years ago
Attachment #607282 - Flags: feedback?(dbaron)
> 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?
Why aren't there infallible versions of EnsureMutable and ReplacePrep?  Seems like that would save a bunch of if statements.
(Assignee)

Comment 6

6 years ago
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.
> 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.
> +    // 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.
Attachment #607282 - Flags: review?(justin.lebar+bug) → review+
Blocks: 742614
(Assignee)

Updated

5 years ago
Depends on: 736501
(Assignee)

Comment 9

5 years ago
Created attachment 619991 [details] [diff] [review]
Move the declaration of fallible_t to its own header, rev. 1
Attachment #619991 - Flags: review?(jones.chris.g)
(Assignee)

Comment 10

5 years ago
Created attachment 619998 [details] [diff] [review]
Move the declaration of fallible_t to its own header, rev. 1.1

Bah, need C++ include guard.
Attachment #619998 - Flags: review?(jones.chris.g)
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 ...
Attachment #619998 - Flags: review?(jones.chris.g)
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
Attachment #619991 - Flags: review?(jones.chris.g) → review+
(Assignee)

Updated

5 years ago
Attachment #607284 - Flags: review?(jduell.mcbugs)
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().
Websockets will no longer slurp whole blob in once bug 704447 is fixed, BTW.
https://hg.mozilla.org/mozilla-central/rev/39e55d53e0a3
https://hg.mozilla.org/mozilla-central/rev/b6d494a57122
https://hg.mozilla.org/mozilla-central/rev/971a0d129ff3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/45ffd4b20e11
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 623066 [details] [diff] [review]
Make ReadInputStreamToString fallible

and here's the patch...
Attachment #623066 - Flags: review?(benjamin)
(Assignee)

Comment 19

5 years ago
Comment on attachment 623066 [details] [diff] [review]
Make ReadInputStreamToString fallible

sorry, I somehow got confused on which patches were reviewed :-(
Attachment #623066 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a88c34de5d01
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?
Created attachment 623357 [details] [diff] [review]
Make ReadInputStreamToString fallible, v2
Attachment #623066 - Attachment is obsolete: true
Attachment #623357 - Flags: review?(benjamin)
(Assignee)

Comment 23

5 years ago
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.
Attachment #623357 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdd1e7c33358
Comment on attachment 607284 [details]
Part B - necko changes, checkpoint

belated +r to already-landed patch
Attachment #607284 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 26

5 years ago
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.
tracking-firefox15: --- → ?
https://hg.mozilla.org/mozilla-central/rev/bdd1e7c33358
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 755323
tracking-firefox15: ? → ---

Updated

5 years ago
Keywords: sec-want
marking for tracking as per comment 26, if there are regressions please update the status flag so this gets on our radar again.
status-firefox15: --- → fixed
tracking-firefox15: --- → +

Updated

5 years ago
Depends on: 767343
Depends on: 770262

Updated

5 years ago
Depends on: 804742
Attachment #607282 - Flags: feedback?(dbaron) → feedback+

Updated

5 years ago
Duplicate of this bug: 383570

Updated

5 years ago
Blocks: 427099
Blocks: 1027163
You need to log in before you can comment on or make changes to this bug.