1058252 - Uninitialised value use in DOMProxyHandler::delete_

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Julian Seward [:jseward]]

test path =
dom/imptests/html/html/dom/elements/global-attributes/test_dataset-delete.html

$objdir/dome/bindings/DOMStringMapBinding.cpp, which seems to be
generated code, has this function, and it appears that control
is arriving at the indicated place without any assignment to |found|.

That could happen if 

* ConvertIdToString sets isSymbol to true 
  and returns non-zero.

or

* ConvertIdToString sets isSymbol is set to false 
  and returns nonzero
  and NamedDeleter() doesn't assign to |found|.

I tried to find out where this code was generated, but failed.  Any
hints?

bool
DOMProxyHandler::delete_(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id, bool* bp) const
{
  MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy),
            "Should not have a XrayWrapper here");

  bool found;
  binding_detail::FakeString name;
  bool isSymbol;
  if (!ConvertIdToString(cx, id, name, isSymbol)) {
    return false;
  }
  if (!isSymbol) {
    nsDOMStringMap* self = UnwrapProxy(proxy);
    self->NamedDeleter(NonNullHelper(Constify(name)), found);
    MOZ_ASSERT(!JS_IsExceptionPending(cx));
  }
  *bp = true;
  if (found) {   <-------------------- HERE == line 419
    return true;
  }

  return dom::DOMProxyHandler::delete_(cx, proxy, id, bp);
}

Comment 1

[Julian Seward [:jseward]]

Resulting V complaint is:

Conditional jump or move depends on uninitialised value(s)
   at 0x630E670: mozilla::dom::DOMStringMapBinding::DOMProxyHandler::delete_(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) const (ff-O-linux64/dom/bindings/DOMStringMapBinding.cpp:419)
   by 0x7CCABBC: js::Proxy::delete_(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) (js/src/jsproxy.cpp:2312)
   by 0x7CDBFC9: js::proxy_DeleteGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) (js/src/jsproxy.cpp:2838)
   by 0x79FC737: JSObject::deleteGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) (js/src/jsobjinlines.h:52)
   by 0x7D7E49D: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2326)
   by 0x7D83071: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:411)
   by 0x7D83772: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:483)
   by 0x7CBDDA3: js_fun_apply(JSContext*, unsigned int, JS::Value*) (js/src/jsfun.cpp:1121)
   by 0x7D8363E: CallJSNative (js/src/jscntxtinlines.h:231)
   by 0x7D8363E: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:464)
   by 0x7D7FA0A: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2546)
   by 0x7D83071: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:411)
   by 0x7D831F6: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) (js/src/vm/Interpreter.cpp:619)
   by 0x7D83321: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (js/src/vm/Interpreter.cpp:656)
   by 0x7C7D593: Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::Value*) (js/src/jsapi.cpp:4783)
   by 0x7C7DBF4: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&) (js/src/jsapi.cpp:4874)
   by 0x61D3CB0: nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) (dom/base/nsJSUtils.cpp:243)

Comment 2

[Boris Zbarsky [:bzbarsky]]

The code is generated by the CGDOMJSProxyHandler_delete class in dom/bindings/Codegen.py.

Looks like this bug was introduced in bug 1041261.

Seems to me like this bit:

  *bp = true;
  if (found) {
    return true;
  }

should be inside the !isSymbol check.

Comment 3

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

And yes, if a symbol comes through here "found" will be used uninitialized.

Comment 5

[Eric Rahm [:erahm]]

This problem shows up in |defineProperty| and |hasOwn| as well. Coverity has identified 43 instances in the most recent last run (2014/10/9).

|hasOwn| snippet:
   
> 6. Condition !mozilla::dom::HasPropertyOnPrototype(cx, proxy, id), taking true branch
> 394  if (!HasPropertyOnPrototype(cx, proxy, id)) {
>   
> 7. var_decl: Declaring variable found without initializer.
> 395    bool found;
> 396    binding_detail::FakeString name;
> 397    bool isSymbol;
>    
> 8. Condition !mozilla::dom::ConvertIdToString(cx, id, name, isSymbol), taking false branch
> 398    if (!ConvertIdToString(cx, id, name, isSymbol)) {
> 399      return false;
> 400    }
>    
> 9. Condition !isSymbol, taking false branch
> 401    if (!isSymbol) {
> 402      mozilla::dom::HTMLFormControlsCollection* self = UnwrapProxy(proxy);
> 403      Nullable<OwningRadioNodeListOrElement> result;
> 404      self->NamedGetter(NonNullHelper(Constify(name)), found, result);
> 405      MOZ_ASSERT(!JS_IsExceptionPending(cx));
> 406      (void)result;
> 407    }
> 408
>    
> CID 1244721 (#1 of 1): Uninitialized scalar variable (UNINIT)10. uninit_use: Using uninitialized value > found.
> 409    *bp = found;
> 410    return true;
> 411  }

|defineProperty| snippet:

> CID 1244706 (#1 of 1): Uninitialized scalar variable (UNINIT)5. uninit_use: Using uninitialized value found.
> 739  if (found) {
> 740    return js::IsInNonStrictPropertySet(cx) || ThrowErrorMessage(cx, MSG_NO_NAMED_SETTER, "TreeColumns");
> 741  }
> 742  return mozilla::dom::DOMProxyHandler::defineProperty(cx, proxy, id, desc, defined);

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

Jason - This bug hasn't seen much progress recently. Are you planning to address this in 34?

Comment 7

[Jason Orendorff [:jorendorff]]

I will try to fix this tomorrow. I looked at it earlier but it's really tricky.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Use of uninitialized value in DOMProxyHandler::delete_

Comment 9

[Jason Orendorff [:jorendorff]]

Never mind, this wasn't so bad once I figured out what the heck was going on in there.

foundVarGiven is a bit ugly. It's needed because if the caller *doesn't* pass a foundVar, CGProxySpecialOperation declares a local `bool found;` anyway and sets foundVar to "found". But in that case, `found` is only in scope inside the `if (!isSymbol)` block, so we must *not* emit an else block that does `found = false;`.

Super annoying that foundVar has different scope in different cases. There should be some kind of comment about this in the code, though I have no idea what it should say...

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Use of uninitialized value in DOMProxyHandler::delete_

Comment 11

[Julian Seward [:jseward]]

Jason's patch does change the generated code, but V is still
complaining of an uninitialised value use, and at the same spot.
Here's what the generated DOMProxyHandler::delete_ now looks like:

  bool
  DOMProxyHandler::delete_(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id, bool* bp) const
  {
    MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy),
              "Should not have a XrayWrapper here");

    bool found;
    binding_detail::FakeString name;
    bool isSymbol;
    if (!ConvertIdToString(cx, id, name, isSymbol)) {
      return false;
    }
    if (!isSymbol) {
      nsDOMStringMap* self = UnwrapProxy(proxy);
      self->NamedDeleter(NonNullHelper(Constify(name)), found);
      MOZ_ASSERT(!JS_IsExceptionPending(cx));
    } else {
      found = false;
    }
    *bp = true;
    if (found) {
      return true;
    }

    return dom::DOMProxyHandler::delete_(cx, proxy, id, bp);
  }

AFAICS the effect of Jason's patch is to add the "else { found = false; }"
bit.  But the callee self->NamedDeleter still doesn't always assign to
|found|:

  void
  nsDOMStringMap::NamedDeleter(const nsAString& aProp, bool& found)
  {
    // Currently removing property, attribute is already removed.
    if (mRemovingProp) {
      return;
    }
    
    nsAutoString attr;
    if (!DataPropToAttr(aProp, attr)) {
      return;
    }
    ...

So .. is nsDOMStringMap::NamedDeleter wrong, or is there some
confusion about the contract between it and DOMProxyHandler::delete_ ?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ouch.  That's totally a bug.  nsDOMStringMap::NamedDeleter should be setting found to false up front.

As far as the patch goes, would it be simpler to just replace all the "bool found;" with "bool found = false;"?  I'd have a lot more faith in that being correct given all the complexity here.  :(

Comment 13

[Julian Seward [:jseward]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> nsDOMStringMap::NamedDeleter should be setting found to false up front.

Filed as bug 1092333.  With that fixed and Jason's patch applied, this
test runs V-clean, so +1 from me for Jason's patch.

Comment 14

[Jason Orendorff [:jorendorff]]

Long story short, bz convinced me to do this the other way, even though I don't like it. I'll fix the bug in NamedDeleter while I'm at it.

Comment 15

[Jason Orendorff [:jorendorff]]

> I'll fix the bug in NamedDeleter while I'm at it.

Nope, Julian already fixed it in bug 1092333.

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: Use of uninitialized value in DOMProxyHandler::delete_

Comment 17

[Jason Orendorff [:jorendorff]]

Character notes on this patch and me:

1. This patch looks even nicer than I thought it would

2. I like it even less than I thought I would

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8517048 [details] [diff] [review]
Use of uninitialized value in DOMProxyHandler::delete_

r=me.  This makes me much more certain that we're not missing a codepath.  :(

Comment 19

[Julian Seward [:jseward]]

(In reply to Jason Orendorff [:jorendorff] from comment #17)
> 2. I like it even less than I thought I would

As a random drive-by comment / my-2-euro-cents etc, I'd say that
easily 1/3 of the bugs I pick up in Valgrind runs of
mochitests/whatever are of the form "such-and-such fn didn't write to
its out parameter(s), when the caller expected it should have done".
So I am all in favour of the initialise-the-out-parameters-by-default
kind of approach.

Comment 20

[Jason Orendorff [:jorendorff]]

(In reply to Julian Seward [:jseward] from comment #19)
> As a random drive-by comment / my-2-euro-cents etc, I'd say that
> easily 1/3 of the bugs I pick up in Valgrind runs of
> mochitests/whatever are of the form "such-and-such fn didn't write to
> its out parameter(s), when the caller expected it should have done".
> So I am all in favour of the initialise-the-out-parameters-by-default
> kind of approach.

When (as in this case) the variable has a kind of natural default that we consider safer, sure.

In other cases, pre-initializing outparams is just (a) replacing a kind of error that we can detect automatically with one that we can't; and (b) replacing the explicit indication of the outparam value in every path with implicit defaulting in some paths. (a) is bad for correctness and security; (b) is bad for the ability to reason about code.

I swear I wish we had someone with the time and interest to pursue static tooling. I want outparam analysis for this kind of bug, and I want assurance that error returns are checked for and handled properly. I *love* Valgrind, it's a life saver, but Valgrind is an awfully expensive way to go about finding this bug.

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to Jason Orendorff [:jorendorff] from comment #20)
> (In reply to Julian Seward [:jseward] from comment #19)
> > As a random drive-by comment / my-2-euro-cents etc, I'd say that
> > easily 1/3 of the bugs I pick up in Valgrind runs of
> > mochitests/whatever are of the form "such-and-such fn didn't write to
> > its out parameter(s), when the caller expected it should have done".
> > So I am all in favour of the initialise-the-out-parameters-by-default
> > kind of approach.
> 
> When (as in this case) the variable has a kind of natural default that we
> consider safer, sure.
> 
> In other cases, pre-initializing outparams is just (a) replacing a kind of
> error that we can detect automatically with one that we can't; and (b)
> replacing the explicit indication of the outparam value in every path with
> implicit defaulting in some paths. (a) is bad for correctness and security;
> (b) is bad for the ability to reason about code.
> 
> I swear I wish we had someone with the time and interest to pursue static
> tooling. I want outparam analysis for this kind of bug, and I want assurance
> that error returns are checked for and handled properly.

What does your ideal static analysis here look like?  You want to enforce:

- Along all paths that we return a success value, outparam X is initialized?
- Callers must not touch outparams if callees return failure?

MSVC can check for these sorts of things, at the cost of adding horrendous annotations to parameters.

Agreed that we should invest more in this sort of stuff.

Comment 22

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24f024e7cbce

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24f024e7cbce

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Jason/bz - Now that we have a fix, do we NEED to take this in 34 or can we safely let this ride the 35 train?

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'm not sure we need this on 34 (and note that it's not on 35!), but we definitely want bug 1092333 on both branches.

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Sounds good. My comment about 35 was meant to be that we can uplift to 35 (still on Aurora) if that's wanted. If so, let's get that done before the uplift to beta.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this missed the initial cut to Beta and bug 1092333 landed in 35, removing tracking here and it can ride in 36.