If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

MozPromise's InvokeAsync dangerously stores references as-is

RESOLVED FIXED in Firefox 52

Status

()

Core
XPCOM
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

50 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
InvokeAsync uses automatic template type deduction to store arguments in the dispatched task.
This means that if it is given an argument that is a reference, it will be stored as a reference, and in the worst case the real object could be destroyed by the time the target function is called, causing UAF.

For simplicity we should just drop references when storing arguments.
For cases where a reference really would have worked (e.g. an object that is certain to outlive the dispatch), then it should be easy enough to convert it to a pointer.
There are not that many uses of InvokeAsync (yet), so it should be possible to have a look at all existing calls and decide if they need to be modified.

Alternatively, or in a later bug, we could use a more powerful system, like that used for NewRunnableMethod.

(I'm leaving this bug unassigned for now, if someone else wants to tackle it before I can find the time.)

Comment 1

a year ago
Auto-deducing argument types for async methods is intrinsically dangerous. That's why we require explicit template typing for NewRunnableMethod. I think we should require the same here.
Comment hidden (mozreview-request)
(Assignee)

Comment 3

11 months ago
(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Auto-deducing argument types for async methods is intrinsically dangerous.
> That's why we require explicit template typing for NewRunnableMethod. I
> think we should require the same here.
I didn't try to import/reuse the whole NewRunnableMethod storage capabilities here, so in the meantime I'm just adding a static_assert that will catch any attempt to pass references.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=770c82d87429452d3b5a79373f4b1c8b52fd8f83
Assignee: nobody → gsquelart

Comment 4

11 months ago
mozreview-review
Comment on attachment 8804996 [details]
Bug 1300476 - Prevent passing references through InvokeAsync -

https://reviewboard.mozilla.org/r/88804/#review88178

At first I thought, "yes, of course, r+" and then I thought, "wait, no, let's think about this" and then I thought "well, that'd be a lot of work."  So I'm on the fence, but tilting towards r+; mozreview doesn't give me anyway to say "r+0.5", so I'm setting r+, but I'd like to have a small discussion about this first.

::: xpcom/threads/MozPromise.h:996
(Diff revision 1)
> +  static_assert(!detail::Any(IsReference<ArgTypes>::value...),
> +                "Cannot pass reference types through InvokeAsync");

This seems like an expedient but limiting solution.  It's not technically incorrect for the the method itself to take references, but it would be incorrect for the runnable to have reference-valued member variables.  So I think this solution only works because `MethodCallType` doesn't care about `ActualArgTypes`, but only `ArgTypes`, and since we've verified that `ArgTypes` is reference-free, things effectively get copied and/or moved instead of `MethodCall` storing references?  Is that correct?

There are two ways of doing what we want here.  The first is to duplicate what `NewRunnableMethod` and friends do, and require explicit template types for `ActualArgTypes`, which would then get passed through to `MethodCall`.  The second is to adopt the `WrapRunnable` approach, but to be smarter about it: pass `ActualArgTypes` through to `MethodCall`, but to define `mArgs` thusly:

```
  Tuple<typename Decay<ActualArgTypes>::Type...> mArgs;
```

and thus effectively capture any arguments we need by copying them.

I know some people truly loathe having to provide explicit template argument types.  They do, however, provide some measure of security that implicit argument types can't provide.  OTOH, I think all the stuff `NewRunnableMethod` does under the hood with the template types you supply could be applied to implicit template argument types, so we could get the best of both worlds: safety and terseness.

On the third hand, I'm not keen on requiring you to rewrite a whole bunch of stuff just so we can make this check slightly more correct; reference arguments tend to be rare in our codebase anyway...
Attachment #8804996 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
Comment on attachment 8804996 [details]
Bug 1300476 - Prevent passing references through InvokeAsync -

https://reviewboard.mozilla.org/r/88804/#review88178

> This seems like an expedient but limiting solution.  It's not technically incorrect for the the method itself to take references, but it would be incorrect for the runnable to have reference-valued member variables.  So I think this solution only works because `MethodCallType` doesn't care about `ActualArgTypes`, but only `ArgTypes`, and since we've verified that `ArgTypes` is reference-free, things effectively get copied and/or moved instead of `MethodCall` storing references?  Is that correct?
> 
> There are two ways of doing what we want here.  The first is to duplicate what `NewRunnableMethod` and friends do, and require explicit template types for `ActualArgTypes`, which would then get passed through to `MethodCall`.  The second is to adopt the `WrapRunnable` approach, but to be smarter about it: pass `ActualArgTypes` through to `MethodCall`, but to define `mArgs` thusly:
> 
> ```
>   Tuple<typename Decay<ActualArgTypes>::Type...> mArgs;
> ```
> 
> and thus effectively capture any arguments we need by copying them.
> 
> I know some people truly loathe having to provide explicit template argument types.  They do, however, provide some measure of security that implicit argument types can't provide.  OTOH, I think all the stuff `NewRunnableMethod` does under the hood with the template types you supply could be applied to implicit template argument types, so we could get the best of both worlds: safety and terseness.
> 
> On the third hand, I'm not keen on requiring you to rewrite a whole bunch of stuff just so we can make this check slightly more correct; reference arguments tend to be rare in our codebase anyway...

As I wrote in comment 3, "in the meantime" ;-)

I intend to add more intelligence in there, but since it would be on the more difficult side (and not that urgent by itself), I thought I would take care of the urgent issue right now, that is removing the current risk of using references until we get a better&safer system.

I will open a follow-up bug with Benjamin's and your suggestions. And Jean-Yves' too, who suggested only requiring explicit argument types *if* one of them is a reference.

And since this is a static_assert, it's intended for developers, so I will mention the new bug in the assertion message, in case an annoyed developer really needs reference-passing, then they can go ahead and code it!
(Assignee)

Updated

11 months ago
Blocks: 1313497
Comment hidden (mozreview-request)

Comment 7

11 months ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/54ad8aaf59c7
Prevent passing references through InvokeAsync - r=froydnj

Comment 8

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/54ad8aaf59c7
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → bug 1361263
You need to log in before you can comment on or make changes to this bug.