Closed Bug 519926 Opened 11 years ago Closed 11 years ago

Null dereference in [@xpc_qsDOMString::xpc_qsDOMString] leads to crash [@ xpc_qsDOMString::xpc_qsDOMString(JSContext*, int, int*, xpc_qsDOMString::StringificationBehavior, xpc_qsDOMString::StringificationBehavior)]

Categories

(Core :: XPConnect, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: m8r-mui2oa, Assigned: m8r-mui2oa)

References

Details

(Keywords: crash, regression, topcrash, Whiteboard: [#3 trunk (1.9.3a1pre) topcrash])

Crash Data

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 

## Premise ##
We start with trying to execute the following piece of JavaScript:
    document.write();

http://hg.mozilla.org/mozilla-central/file/2144fdf97e50/js/src/jsops.cpp#l2235
In function js_Interpret() of /js/src/jsops.cpp:
    ok = ((JSFastNative) fun->u.n.native)(cx, argc, vp);

This will invoke the quick stub generated for nsIDOMHTMLDocument.write().
Note that we provided zero arguments to document.write(), so argc will be zero.


## New circumstances ##
The next thing to move on is to examine nsIDOMHTMLDocument_Write(), which will
be automatically generated by qsgen.py according to the its interface:

http://hg.mozilla.org/mozilla-central/file/2144fdf97e50/dom/interfaces/html/nsIDOMHTMLDocument.idl
In /dom/interfaces/html/nsIDOMHTMLDocument.idl:
    void write([optional, Null(Stringify)] in DOMString text);

Please note the Null(Stringify). It specifies that the browser should serialize
JS_NULL as the string "null". Support for this IDL annotation was recently
implemented, please see:
    Bug 478251
    Changeset http://hg.mozilla.org/mozilla-central/rev/39b6673348d2


## How this effects the generated source ##
(You can skip this paragraph if you have access to a working build system and
 simply look into /build/obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp)

http://hg.mozilla.org/mozilla-central/file/2144fdf97e50/js/src/xpconnect/src/qsgen.py#l446
The function writeQuickStub() in /js/src/xpconnect/src/qsgen.py will call
writeArgumentUnboxing() for each argument defined in the IDL with i set to the
IDL argument index.

Now, let's try to fill in the blanks. We have:
    cx, argc=0, vp                             (supplied by the JS interpreter)
    optional=True, nullBehavior=eStringify     (from the IDL)
    undefinedBehavior=eStringify               (from eDefaultUndefinedBehavior)

For your reference, here's the template for the conversion to [domstring]:
    xpc_qsDOMString ${name}(cx, ${argVal}, ${argPtr},
        xpc_qsDOMString::e${nullBehavior},
        xpc_qsDOMString::e${undefinedBehavior})

Since we know that optional is True, argVal and argPtr will be set to
    elif optional:
        argVal = "(%d < argc ? argv[%d] : JSVAL_NULL)" % (i, i)
        argPtr = "(%d < argc ? &argv[%d] : NULL)" % (i, i)

This results in the following call being written into dom_quickstubs.cpp:
    xpc_qsDOMString arg0(cx, (0 < argc ?  argv[0] : JSVAL_NULL),
                             (0 < argc ? &argv[0] :       NULL),
                             eStringify, eStringify);

Knowing that argc is 0 in our example, we can simplify this to:
    xpc_qsDOMString arg0(cx, JSVAL_NULL, NULL, eStringify, eStringify);


## Where the problem arises ##
http://hg.mozilla.org/mozilla-central/file/2144fdf97e50/js/src/xpconnect/src/xpcquickstubs.cpp#l722
As implied, the constructor will be called next, which is the point of failure:
    xpc_qsDOMString::xpc_qsDOMString(JSContext *cx, jsval v, jsval *pval,
                                     StringificationBehavior nullBehavior,
                                     StringificationBehavior undefinedBehavior)

Argument v is subsequently checked with JSVAL_IS_NULL and JSVAL_IS_VOID,
causing the behavior to be set to either nullBehavior or undefinedBehavior.
In our example, this case will be executed:
    if(JSVAL_IS_NULL(v)) {
        behavior = nullBehavior;
	} else [...]

This ultimately means, the behavior will be set to eStringify, as specified in
the argument and the directly following
    if (behavior != eStringify) { [...]; return; }
construct will not prevent us from going the deadly path following ahead:
    *pval = STRING_TO_JSVAL(s);


## The implications ##
1. Calling the xpc_qsDOMString constructor with
      a) v == JSVAL_NULL and pval == NULL and      nullBehavior == eStringify
      b) v == JSVAL_VOID and pval == NULL and undefinedBehavior == eStringify
   will ALWAYS cause a null pointer dereference.

2. These conditions can be fulfilled by:
      1) Passing no arguments to a DOM function implemented as quick stub
      2) Specifying Null(Stringify) for an "optional" argument in the IDL

I think case 1b) is unfulfillable by this code path since as soon as undefined
is passed, argc would rise to at least 1.


## Listing of affected DOM functions ##
Determined via egrep -ir null\(stringify\) mozilla-central:
    nsIDOMHTMLDocument.write()
    nsIDOMHTMLDocument.writeln()
Only the trunk is affected by this bug.


## What needs to be done ##
1. xpc_qsDOMString() must not attempt to stringify if pval is invalid.
2. The automated test for Bug 478251 should be updated to include this bug.
   It is located at /content/html/document/test/test_bug478251.html
   (Is this correct? Or should this be a seperate crashtest?)


## References ##
http://forums.mozillazine.org/viewtopic.php?f=23&t=1507095
http://crash-stats.mozilla.com/report/index/fbe8ebf2-88f6-4771-8bc8-59ccc2090927?p=1


Reproducible: Always
Attached file Test case, valid XHTML
Attachment #403989 - Flags: review?
Keywords: crash, regression
Version: unspecified → Trunk
Good catch!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 403989 [details] [diff] [review]
Proposed patch for xpcquickstubs.cpp

Zelgadis, when you request review, you have to request review from a specific person. The easiest way to see who to request review from is to do 'hg log <filename>' and see who works/reviews in the file.

In this case jorendorff is the right guy to look at this.
Attachment #403989 - Flags: review? → review?(jorendorff)
Duplicate of this bug: 520223
Whiteboard: [#3 trunk (1.9.3a1pre) topcrash]
There are two bugs here.

I feel I have a definite understanding of what [optional] and [Null(Stringify)] are supposed to mean. Stop me if I go wrong:

  * [optional] means that if the argument is omitted,
    we behave as if the caller had passed JS `null`.

  * [Null(Stringify)] means that if we receive JS `null`,
    we behave as if the caller had passed the string "null".

So our IDL appears to say that document.write() without arguments should call nsIDOMHTMLDocument::Write passing the string "null". This is not what we want, so I think our IDL must be wrong. That's bug #1.

However, if we *did* have a quickstubbed method and we really did want it to have an [optional, Null(Stringify)] argument, and the caller omitted it, we would want the quick stub to pass the string "null", not crash. That's bug #2.

The patch only tries to address bug #2; and it's not the right fix, because the resulting behavior is to create a void string, not the string "null".

Cc:ing bz and peterv because it would be best to fix both bugs at once. I hope bz can take this one.
I think Null(stringify) should just be ignored for optional arguments that are not in fact passed in.  At least for the time being.  That seems to be more or less the idea of the patch, right?
(In reply to comment #8)
If "null" is the desired output for not passing anything at all, then my patch
is not doing that - your're completely right about this.

I didn't have the same understanding of how [optional] is supposed to behave;
in fact I just read that whole code for the first time and drew the conclusion
that [optional] means "may or may not be filled" rather than "defaults to null
if nothing is here".

(In reply to comment #9)
Yes, that was the idea, but since there seems to be only one code path leading
into xpc_qsDOMString::xpc_qsDOMString, it'd also make sense to fix it at a
higher level instead. Either way would fix the crash but as jorrendorf said,
mine would produce a void string since there's no space allocated at pval where
we could write "null" into.
Duplicate of this bug: 521115
Taking bug to track it; will reassign to patch author when I push it.
Assignee: nobody → bzbarsky
Attachment #403989 - Attachment is obsolete: true
Attachment #403990 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #405197 - Flags: review?(jorendorff)
Attachment #403989 - Flags: review?(jorendorff)
So to make it clear, the XPConnect behavior for this idl if no string is passed it to create a void nsAString.  So is the quickstubs behavior with this patch.  The behavior if explicit |null| is passed is different, since xpconnect doesn't understand the Null annotation.  And the general behavior of this method can no longer be expressed in XPConnect.
Duplicate of this bug: 521760
Summary: Null dereference in [@xpc_qsDOMString::xpc_qsDOMString] leads to crash → Null dereference in [@xpc_qsDOMString::xpc_qsDOMString] leads to crash [@ xpc_qsDOMString::xpc_qsDOMString(JSContext*, int, int*, xpc_qsDOMString::StringificationBehavior, xpc_qsDOMString::StringificationBehavior)]
Attachment #405197 - Flags: review?(jorendorff) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/cb0f0cbac15c
Assignee: bzbarsky → zelgadis
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Crash Signature: [@xpc_qsDOMString::xpc_qsDOMString] [@ xpc_qsDOMString::xpc_qsDOMString(JSContext*, int, int*, xpc_qsDOMString::StringificationBehavior, xpc_qsDOMString::StringificationBehavior)]
You need to log in before you can comment on or make changes to this bug.