1066846 - Calling nsIConverterInputStream.readString with uninitialized var crashes firefox

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Dave Hylands [:dhylands]]

STR:
- create an empty text file. I was running under linux, so I did
> touch /home/dhylands/somefile.txt

- Bring up Tools->Web Developer->Web Console
- Click on settings (gear icon) and check "Enable chrome and add-on debugging"
- Bring up Tools->Web Developer->Browser Console

Paste the following into the input text area:

let lockFile = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsIFile);
let foStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
let coStream = Cc["@mozilla.org/intl/converter-input-stream;1"].createInstance(Ci.nsIConverterInputStream);
lockFile.initWithPath('/home/dhylands/somefile.txt');
foStream.init(lockFile, -1, 0, 0);
coStream.init(foStream, "UTF-8", 0, 0);
let str;
coStream.readString(-1, str);

When you press RETURN, then the browser will crash. I originally observed this on FirefoxOS as part of some chrome code. That was on trunk. I also reproduced in Aurora on desktop.

If I use:
let str = {};

then it executes fine.

Comment 1

[Dave Hylands [:dhylands]]

https://crash-stats.mozilla.com/report/index/ca58c741-cae2-448e-b980-feac82140912

Comment 2

[Boris Zbarsky [:bzbarsky]]

readString is defined like so in the IDL:

     unsigned long readString(in unsigned long aCount, out AString aString);

which means it expects an object for the outparam.

The crashing xpconnect code looks like this (in XPCWrappedNative.cpp, CallMethodHelper::GatherAndConvertResults):

  1939             // we actually assured this before doing the invoke
  1940             MOZ_ASSERT(mArgv[i].isObject(), "out var is not object");
  1941             RootedObject obj(mCallContext, &mArgv[i].toObject());
  1942             if (!JS_SetPropertyById(mCallContext, obj, mIdxValueId, v)) {

The comment there is obviously wrong, since mArgv[i] is in fact an UndefinedValue.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the issue is that in CallMethodHelper::ConvertIndependentParam the relevant param is marked as in + dipper.  It's also IsStringClass.

So we end up doing the AllocateStringClass bit and then returning true because IsDipper.  So we never call CallMethodHelper::GetOutParamSource which is what checks for all non-string arguments that we got handed an actual object.

Looks to me like this got broken in bug 683802.  Before that dipper types made it down to GetOutParamSource....

Comment 4

[Bobby Holley (:bholley)]

Attachment: Explicitly verify that incoming dipper parameters are objects. v1

Comment 5

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=9234dfad3536

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489060 [details] [diff] [review]
Explicitly verify that incoming dipper parameters are objects. v1

>+            if (i < mArgc && !mArgv[i].isObject())
>+                ThrowBadParam(NS_ERROR_XPC_NEED_OUT_OBJECT, i, mCallContext);

Don't you need a return false somewhere in there?

r=me with that fixed.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Don't you need a return false somewhere in there?

Doh, yes. Don't know how I missed that.

Comment 8

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f91c37926ae8

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f91c37926ae8