583699 - nsGlobalWindow::Prompt passes uninitialised INOUT param to nsIPromptService::Prompt

Status: RESOLVED WORKSFORME

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

Description

[Julian Seward [:jseward]]

TEST_PATH=editor/libeditor/text/tests/test_bug569988.html


dom/base/nsGlobalWindow.cpp, function nsGlobalWindow::Prompt:

4283:  PRBool ok, dummy;
4284:  rv = promptSvc->Prompt(this, title.get(), fixedMessage.get(),
4285:                         &inoutValue, nsnull, &dummy, &ok);

I believe it is calling this

NS_SCRIPTABLE NS_IMETHOD Prompt(nsIDOMWindow *aParent, 
                                const PRUnichar *aDialogTitle, 
                                const PRUnichar *aText, 
                                PRUnichar **aValue NS_INOUTPARAM, 
                                const PRUnichar *aCheckMsg, 
                                PRBool *aCheckState NS_INOUTPARAM, 
                                PRBool *_retval NS_OUTPARAM) = 0;

*aCheckState is marked as NS_INOUTPARAM but the caller passes 'dummy'
uninitialised.  Resulting complaints:




Conditional jump or move depends on uninitialised value(s)
   at 0x5DD5E46: XPCVariant::InitializeData (xpcvariant.cpp:307)
   by 0x5DD6554: XPCVariant::newVariant (xpcvariant.cpp:148)
   by 0x5DC4C6C: XPCConvert::JSData2Native (xpcconvert.cpp:977)
   by 0x5DDF46C: XPCWrappedNative::CallMethod (xpcwrappednative.cpp:2909)
   by 0x5DE812D: XPC_WN_CallMethod (xpcwrappednativejsops.cpp:1796)
   by 0x6F8F753: Invoke (jscntxtinlines.h:339)
   by 0x6F8FCDF: js_Invoke (jsinterp.cpp:693)
   by 0x6F80196: js_Interpret (jsops.cpp:2155)
   by 0x6F8F8B6: Invoke (jsinterp.cpp:602)
   by 0x6F8FCDF: js_Invoke (jsinterp.cpp:693)
   by 0x5DDAA74: nsXPCWrappedJSClass::CallMethod (xpcwrappedjsclass.cpp:1689)
   by 0x62BBBFF: PrepareAndDispatch (xptcstubs_x86_64_linux.cpp:153)
   by 0x62BB0CA: SharedStub (in .../ff-opt/toolkit/library/libxul.so)
   by 0x5AE0514: nsGlobalWindow::Prompt (nsGlobalWindow.cpp:4285)
   by 0x62BAF76: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:208)
   by 0x5DDFA37: XPCWrappedNative::CallMethod (xpcwrappednative.cpp:3063)
   by 0x5DE812D: XPC_WN_CallMethod (xpcwrappednativejsops.cpp:1796)
   by 0x6F8F753: Invoke (jscntxtinlines.h:339)
   by 0x6F8FCDF: js_Invoke (jsinterp.cpp:693)
   by 0x6F80196: js_Interpret (jsops.cpp:2155)
 Uninitialised value was created by a stack allocation
   at 0x5AE03B0: nsGlobalWindow::Prompt (nsGlobalWindow.cpp:4255)


then click on 'OK' in the popup, and:


Conditional jump or move depends on uninitialised value(s)
   at 0x6F4F3C7: js_ValueToBoolean (jsbool.cpp:177)
   by 0x6F3E87B: JS_ValueToBoolean (jsapi.cpp:496)
   by 0x5DC469A: XPCConvert::JSData2Native (xpcconvert.cpp:592)
   by 0x5DD9DB9: nsXPCWrappedJSClass::CallMethod (xpcwrappedjsclass.cpp:1781)
   by 0x62BBBFF: PrepareAndDispatch (xptcstubs_x86_64_linux.cpp:153)
   by 0x62BB0CA: SharedStub (in .../ff-opt/toolkit/library/libxul.so)
   by 0x5AE0514: nsGlobalWindow::Prompt (nsGlobalWindow.cpp:4285)
   by 0x62BAF76: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:208)
   by 0x5DDFA37: XPCWrappedNative::CallMethod (xpcwrappednative.cpp:3063)
   by 0x5DE812D: XPC_WN_CallMethod (xpcwrappednativejsops.cpp:1796)
   by 0x6F8F753: Invoke (jscntxtinlines.h:339)
   by 0x6F8FCDF: js_Invoke (jsinterp.cpp:693)
   by 0x6F80196: js_Interpret (jsops.cpp:2155)
   by 0x6F8F8B6: Invoke (jsinterp.cpp:602)
   by 0x6F8FCDF: js_Invoke (jsinterp.cpp:693)
   by 0x6F9061B: js_InternalInvoke (jsinterp.cpp:739)
   by 0x6F40A29: JS_CallFunctionValue (jsapi.cpp:4850)
   by 0x5ABC94A: nsJSContext::CallEventHandler (nsJSEnvironment.cpp:2224)
   by 0x5AD14F0: nsGlobalWindow::RunTimeout (nsGlobalWindow.cpp:8506)
   by 0x5AD1930: nsGlobalWindow::TimerCallback (nsGlobalWindow.cpp:8851)
 Uninitialised value was created by a stack allocation
   at 0x5AE03B0: nsGlobalWindow::Prompt (nsGlobalWindow.cpp:4255)

Comment 1

[Julian Seward [:jseward]]

Attachment: Trivial fix

No idea if 'false' is a suitable initial value for 'dummy'.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the issue here is that this is an inout parameter that is unused by the actual callee, and the interface promises that it will be unused.  The warnings come from the marshalling code, which converts the C++ boolean into a JS value and back.

Initializing the value in this case is a bit misleading, since it can make someone think the value matters...

Comment 3

[Julian Seward [:jseward]]

It might be a false positive, I don't know.  There's a lot of
under-the-hood sophistication in Memcheck to avoid them, and they are
pretty rare, but maybe the bit twiddling going on w.r.t. jsvals is
fooling it.

One place Memcheck is complaining is

  XPCVariant::XPCVariant(XPCCallContext& ccx, jsval aJSVal)
      : mJSVal(aJSVal)
  {
      nsVariant::Initialize(&mData);
      if(!JSVAL_IS_PRIMITIVE(mJSVal))    <-----------------------HERE
      {
          // If the incoming object is an XPCWrappedNative, then it could be a
          // double-wrapped object, and we should return the double-wrapped
          // object back out to script.

          JSObject* proto;
          XPCWrappedNative* wn =
              XPCWrappedNative::GetWrappedNativeOfJSObject(ccx,
                                            JSVAL_TO_OBJECT(mJSVal),
                                                           nsnull,
                                                           &proto);
          mReturnRawObject = !wn && !proto;
      }
      else
          mReturnRawObject = JS_FALSE;
  }

Printing the sequence of mJSVal values as 64-bit ints gives a bunch of
fully-defined values like this:

                                          (0 = defined, 1 = undefined)
  mJSVal= fff9000000000000   definedness= 0000000000000000
  mJSVal= fffb80001df1f750   definedness= 0000000000000000
  mJSVal= fff9000000000000   definedness= 0000000000000000
  ...
  mJSVal= fff9000000000000   definedness= 0000000000000000
  mJSVal= fffa800016024280   definedness= 0000000000000000
  mJSVal= fffa80001df08da0   definedness= 0000000000000000
  mJSVal= fffa80001b24b2c0   definedness= 0000000000000000
  mJSVal= fffa80001b24b300   definedness= 0000000000000000
  mJSVal= fffb000000000000   definedness= 0000000000000000

but then

  mJSVal= fff9800000000007   definedness= 00000000ffffffff

so the lowest 32 bits are undefined.  So .. is a JSVal of the
form fff9'8000'xxxx'xxxx unambiguously a Bool?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hrm...  the new jsval stuff is .. confusing.  ;)  Is this on a 32-bit machine, or 64-bit?  And I assume little-endian?

Comment 5

[Julian Seward [:jseward]]

Yes, sorry, this is x86_64.  I got as far as figuring out that

     JSVAL_TAG_BOOLEAN << JSVAL_TAG_SHIFT
  == (JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_BOOLEAN) << JSVAL_TAG_SHIFT
  == 0x1FFF0 | 3ULL) << 47
  == 0xfff9'8000'0000'0000

so the tag looks OK.  Quite how Memcheck managed to divine that all
the payload bits are junk I don't know; perhaps the uninitialised
PRBool in nsGlobalWindow::Prompt was 32-bits.  From looking in
nsprpub/pr/include/prtypes.h it appears that PRBool is a 32-bit int,
so perhaps that's how.

I can't figure out whether or not Memcheck should really have
complained here.  But anyway, given that the noise is confusing ..

> Initializing the value in this case is a bit misleading, since it
> can make someone think the value matters...

.. could you live with a patch that initialises it, plus a comment
explaining that the value doesn't actually matter?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Perhaps the uninitialised PRBool in nsGlobalWindow::Prompt was 32-bits. 

Yes, indeed.

> .. could you live with a patch that initialises it, plus a comment
> explaining that the value doesn't actually matter?

Yes.

Comment 7

[Julian Seward [:jseward]]

Attachment: patch as per comment #6

Comment 8

[Dão Gottwald [:dao]]

needs approval2.0 to land

Comment 9

[Julian Seward [:jseward]]

> needs approval2.0 to land
has been granted.

Comment 10

[Julian Seward [:jseward]]

I found a second instance of this, at
security/manager/ssl/src/nsNSSCallbacks.cpp:837:

836:      PRBool checkState;
837:      rv = proxyPrompt->PromptPassword(nsnull, promptString.get(),
838:                                       &password, nsnull, &checkState, &value);

when running toolkit/components/passwordmgr/test/test_master_password.html
As above, initialising checkState before the call fixes it.



Conditional jump or move depends on uninitialised value(s)
   at 0x5E5055B: XPCVariant::newVariant (jsval.h:673)
   by 0x5E3E5D6: XPCConvert::JSData2Native (xpcconvert.cpp:1003)
   by 0x5E599BC: XPCWrappedNative::CallMethod (xpcwrappednative.cpp:2926)
   by 0x5E629CD: XPC_WN_CallMethod (xpcwrappednativejsops.cpp:1738)
   by 0x64934F4: bool js::InvokeCommon (jscntxtinlines.h:555)
   by 0x6493AAB: js::Invoke (jsinterp.cpp:699)
   by 0x659BF73: js::Interpret (jsinterp.cpp:4709)
   by 0x64935E6: bool js::InvokeCommon (jsinterp.cpp:577)
   by 0x6493AAB: js::Invoke (jsinterp.cpp:699)
   by 0x6486B0E: js_fun_apply (jsfun.cpp:2415)
   by 0x65A4F98: js::Interpret (jsinterp.cpp:4698)
   by 0x64935E6: bool js::InvokeCommon (jsinterp.cpp:577)
   by 0x6493AAB: js::Invoke (jsinterp.cpp:699)
   by 0x64944EF: js::InternalInvoke (jsinterp.cpp:739)
   by 0x6451EAE: JS_CallFunctionValue (jsinterp.h:646)
   by 0x5E53E6B: nsXPCWrappedJSClass::CallMethod (xpcwrappedjsclass.cpp:1687)
   by 0x635935F: PrepareAndDispatch (xptcstubs_x86_64_linux.cpp:153)
   by 0x635882A: SharedStub (in ff-opt/toolkit/library/libxul.so)
   by 0x5FCE076: PK11PasswordPrompt (nsNSSCallbacks.cpp:838)
   by 0xBC36501: PK11_DoPassword (pk11auth.c:535)
   by 0x5FE3F84: nsSecretDecoderRing::Decrypt (nsSDR.cpp:204)
   by 0x5FE3E7A: nsSecretDecoderRing::DecryptString (nsSDR.cpp:269)
   by 0x63586D6: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:208)
   by 0x5E59FCF: XPCWrappedNative::CallMethod (xpcwrappednative.cpp:3080)

 Uninitialised value was created by a stack allocation
   at 0x5FCDB52: PK11PasswordPrompt (nsNSSCallbacks.cpp:753)

Comment 11

[Justin Wood (:Callek)]

This patch has bitrotted, and I'm not sure if its even still a NEW bug anymore based on the function it was in.  Leaving that up to those here though.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 465350 [details] [diff] [review]
patch as per comment #6

(bitrotten)

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This probably crashes now that Bug 658351 has landed ...

Comment 14

[Jonathan Protzenko [:protz]]

The actually changed in the meanwhile, and what used to be a dummy, initialized PRBool is now a properly initialized value.

  PRBool disallowDialog = PR_FALSE;

(...)

  PRBool ok;
  rv = prompt->Prompt(title.get(), fixedMessage.get(),
                      &inoutValue, label.get(), &disallowDialog, &ok);

Kyle, any objections to closing this?

Comment 15

[Jonathan Protzenko [:protz]]

security/manager/ssl/src/nsNSSCallbacks.cpp:837 is covered by bug 662126

Comment 16

[Jonathan Protzenko [:protz]]

Err, bug 659666, sorry, and this has been fixed already.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #14)
> Kyle, any objections to closing this?

No.