1220168 - Untouched outparams get filled with bytes from uninitialized memory

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Here's some sorta-pseudocode:

```C++

#include <stdio.h>

void someFunc(bool* aOutParam) {
}

int main(int argc, char** argv) {
  bool myBool = false;
  someFunc(&myBool);
  if (myBool) {
    printf("Wait, WHAT?");
    return 1;
  }
  return 0;
}
```

Reading that, I'd assume to never enter the conditional that returns 1, since the myBool outparam is never touched. I would expect that myBool will remain false.

In bug 1217190, I had assumed that outparams passed over IPC would behave the same way - that if they weren't touched by the receiver, then they'd still have the values that the sender had initialized them to.

This is not the case.

Here's the generated code for PPrintingParent.cpp that bug 1217190 was caused by:

```C++

            bool notifyOnOpen;
            bool success;
            int32_t id__ = mId;
            if ((!(RecvShowProgress(mozilla::Move(browser), mozilla::Move(printProgressDialog), mozilla::Move(isForPrinting), (&(notifyOnOpen)), (&(success)))))) {
                mozilla::ipc::ProtocolErrorBreakpoint("Handler for ShowProgress returned error code");
                return MsgProcessingError;
            }

            reply__ = new PPrinting::Reply_ShowProgress(id__);

            Write(notifyOnOpen, reply__);
            Write(success, reply__);
            (reply__)->set_sync();
            (reply__)->set_reply();

```

PrintingParent's RecvShowProgress originally did nothing with notifyOnOpen in certain error cases (there is an expected error case on OS X, where we don't actually show a progress window when printing).

The notifyOnOpen bool in the generated code is uninitialized, and then its value was getting written to the reply back to the sender.

This seems like kind of a footgun, especially for people like me who aren't used to digging into the generated C++ that the IPDL parser creates.

ehsan thought it might be useful in debug builds to initialize outparams with a canary value, like 0xDEADBEEF or 0xDECEA5ED, and then after calling the receiver method, to check for those canary values, and explode if we see they were never written to.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, IPDL expects that if you return true from a Recv method that you have set all outparams to something sane. This is the same convention we have with XPCOM methods (you have to set outparams before returning NS_OK).

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #0)
> ehsan thought it might be useful in debug builds to initialize outparams
> with a canary value, like 0xDEADBEEF or 0xDECEA5ED, and then after calling
> the receiver method, to check for those canary values, and explode if we see
> they were never written to.

That seems kind of bad if those are actual valid values of the type.  And for types that do have unused bit patterns (e.g., bool represented as uint8_t), trying to trick the compiler like that is also not the best idea.

Changing IPDL outparams to a smart pointer type and updating every implementation of all 360 IPDL methods that have a `returns` to use the new type might be feasible, but that's a lot of patch for maybe not a huge amount of benefit.

Static checking might be able to catch simple mistakes like bug 1217190 without a huge amount of work or false positives — if there's a path from the entry to a `return true` (or, almost certainly, to a non-constant return) that never even mentions an outparam, then that's a bug.  This should also work for XPCOM outparams.  Getting the methods annotated is harder, but they all inherit from autogenerated interface classes.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I think we have some kind of clang-provided static analysis going on, and I wonder if we could extend it to catch this particular case a build-time error.

:mystor, I seem to recall you working on the clang static analysis stuff... is such static analysis within the realm of near-term possibility?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> I think we have some kind of clang-provided static analysis going on, and I
> wonder if we could extend it to catch this particular case a build-time
> error.
> 
> :mystor, I seem to recall you working on the clang static analysis stuff...
> is such static analysis within the realm of near-term possibility?

Yes, ensuring that an outparam is assigned to over all paths is a near-term possibility, but will still take a while to get working.

We'd probably need to annotate outparams on declarations with MOZ_OUT or something like that, I'd then write an analysis which walks functions with MOZ_OUT parameters, and ensures that either they are passed to another MOZ_OUT parameter, or that they are assigned to (with *out = ...). I could use the same control flow logic I use in bug 1186706.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Does comment 4 sound reasonable to you? Is asking platform hackers to annotate all outparams with MOZ_OUT a reasonable thing to ask?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> Does comment 4 sound reasonable to you? Is asking platform hackers to
> annotate all outparams with MOZ_OUT a reasonable thing to ask?

I should mention that all XPIDL-generated interfaces can just add the MOZ_OUT annotation implicitly in the code generator, avoiding the need for annotating all of the C++ implementers.

Comment 7

[(no longer active)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> Does comment 4 sound reasonable to you? Is asking platform hackers to
> annotate all outparams with MOZ_OUT a reasonable thing to ask?

Yes, it sounds great to me (although I think MOZ_OUT_PARAM might be a better name)!  Since the IPDL protocol classes are all codegen'ed it should be easy to spray over the annotations.  But note that we currently only run those checks on Mac and Linux, so code specific to Windows, Android or b2g won't benefit from this.

One thing that would be hard in static analysis is the question of conditional initialization of outparams.  For example, I think according to the IPDL rules, a method that returns false can leave its out param initialized, and we can't correctly deal with cases like:

bool Class::IPDLMethod(bool* aResult)
{
  if (SomeCondition()) {
    *aResult = true;
    return true;
  }
  return false;
}

So code like this will need to be modified to initialize *aResult unconditionally.  I don't know how prevalent this pattern is, but this is definitely doable (although it may turn into a lot of work.)  This is definitely going to be a huge pain in XPIDL methods, so I wouldn't worry about extending the analysis to cover those just yet.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to [Vacation: Nov 3-19] from comment #7)
> One thing that would be hard in static analysis is the question of
> conditional initialization of outparams.  For example, I think according to
> the IPDL rules, a method that returns false can leave its out param
> initialized, and we can't correctly deal with cases like:
> 
> bool Class::IPDLMethod(bool* aResult)
> {
>   if (SomeCondition()) {
>     *aResult = true;
>     return true;
>   }
>   return false;
> }

I'd been assuming a simple flow analysis, computing which outparams must have been used before each basic block, and ignoring false/non-NS_OK returns.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jed Davis [:jld] from comment #8)
> (In reply to [Vacation: Nov 3-19] from comment #7)
> > One thing that would be hard in static analysis is the question of
> > conditional initialization of outparams.  For example, I think according to
> > the IPDL rules, a method that returns false can leave its out param
> > initialized, and we can't correctly deal with cases like:
> > 
> > bool Class::IPDLMethod(bool* aResult)
> > {
> >   if (SomeCondition()) {
> >     *aResult = true;
> >     return true;
> >   }
> >   return false;
> > }
> 
> I'd been assuming a simple flow analysis, computing which outparams must
> have been used before each basic block, and ignoring false/non-NS_OK returns.

That's pretty much what I was thinking too. I'd require all control paths which either don't return a constant, or which return the constant NS_OK, to have an assignment to all out parameters. It could be extended in a different way if we need as well.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This looks like it ended up as a feature request for static analysis (and not necessarily specific to IPC).