Closed Bug 737164 Opened 12 years ago Closed 12 years ago

Make strings malloc-infallible by default

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox15 + fixed

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Keywords: sec-want)

Attachments

(6 files, 1 obsolete file)

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&);
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)
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.
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+
Depends on: 736501
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+
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.
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 → ---
and here's the patch...
Attachment #623066 - Flags: review?(benjamin)
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+
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?
Attachment #623066 - Attachment is obsolete: true
Attachment #623357 - Flags: review?(benjamin)
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+
Comment on attachment 607284 [details]
Part B - necko changes, checkpoint

belated +r to already-landed patch
Attachment #607284 - Flags: review?(jduell.mcbugs) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/bdd1e7c33358
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 755323
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.
Depends on: 767343
Depends on: 770262
Depends on: 804742
Attachment #607282 - Flags: feedback?(dbaron) → feedback+
Blocks: 427099
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.