Uninitialised value use in DOMProxyHandler::delete_

RESOLVED FIXED in Firefox 36

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
14 days ago

People

(Reporter: jseward, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {coverity, regression})

Trunk
mozilla36
x86_64
Linux
coverity, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 unaffected, firefox32 unaffected, firefox33 unaffected, firefox34+ wontfix, firefox35+ wontfix, firefox36 fixed)

Details

(Whiteboard: [CID 1244573][CID 1244591][CID 1244593][CID 1244594][CID 1244596][CID 1244598][CID 1244599][CID 1244601][CID 1244604][CID 1244614][CID 1244622][CID 1244624][CID 1244630][CID 1244632][CID 1244638][CID 1244666][CID 1244676][CID 1244690][CID 1244695][CID 124…)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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);
}
(Reporter)

Comment 1

4 years ago
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)
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.
Blocks: 1041261
Keywords: regression
[Tracking Requested - why for this release]:

And yes, if a symbol comes through here "found" will be used uninitialized.
tracking-firefox34: --- → ?
status-firefox31: --- → unaffected
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
status-firefox34: --- → affected
tracking-firefox34: ? → +
(Assignee)

Updated

4 years ago
Assignee: nobody → jorendorff
Duplicate of this bug: 1081416
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);
Keywords: coverity
Whiteboard: [CID 1244573][CID 1244591][CID 1244593][CID 1244594][CID 1244596][CID 1244598][CID 1244599][CID 1244601][CID 1244604][CID 1244614][CID 1244622][CID 1244624][CID 1244630][CID 1244632][CID 1244638][CID 1244666][CID 1244676][CID 1244690][CID 1244695][CID 124…
(Assignee)

Updated

4 years ago
Blocks: 1066322
Jason - This bug hasn't seen much progress recently. Are you planning to address this in 34?
Flags: needinfo?(jorendorff)
(Assignee)

Comment 7

4 years ago
I will try to fix this tomorrow. I looked at it earlier but it's really tricky.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 8

4 years ago
Created attachment 8514625 [details] [diff] [review]
Use of uninitialized value in DOMProxyHandler::delete_
Attachment #8514625 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

4 years ago
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...
(Assignee)

Comment 10

4 years ago
Created attachment 8514935 [details] [diff] [review]
Use of uninitialized value in DOMProxyHandler::delete_
Attachment #8514935 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #8514625 - Attachment is obsolete: true
Attachment #8514625 - Flags: review?(bzbarsky)
(Reporter)

Comment 11

4 years ago
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_ ?
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.  :(
Flags: needinfo?(jorendorff)
(Reporter)

Comment 13

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
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.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 15

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

Nope, Julian already fixed it in bug 1092333.
(Assignee)

Comment 16

4 years ago
Created attachment 8517048 [details] [diff] [review]
Use of uninitialized value in DOMProxyHandler::delete_
Attachment #8517048 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #8514935 - Attachment is obsolete: true
Attachment #8514935 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

4 years ago
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 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.  :(
Attachment #8517048 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 19

4 years ago
(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.
(Assignee)

Comment 20

4 years ago
(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.
(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.
Flags: needinfo?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/24f024e7cbce
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
status-firefox34: affected → wontfix
status-firefox35: --- → affected
status-firefox36: --- → fixed
tracking-firefox35: --- → +
Flags: needinfo?(jorendorff)
Since this missed the initial cut to Beta and bug 1092333 landed in 35, removing tracking here and it can ride in 36.
Flags: needinfo?(jorendorff)
status-firefox35: affected → wontfix
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.