209098 - [FIX] TestParser crashes inside nsLoggingSink

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Mats Palmgren (inactive)]

TestParser crashes inside nsLoggingSink on some input.  It doesn't occur
on the regular regression tests under tests/html so maybe it isn't a major
problem.  It does occur for the file the I'm going to attach (which is a
"wget pull" of http://www.travelocity.com/) though.

The crash is 100% reproducable for me on that file.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

[Mats Palmgren (inactive)]

Attachment: GDB stack from the crash (Linux)

Comment 3

[Mats Palmgren (inactive)]

Attachment: Console output before the crash

There were a few assertions before the crash but they could be unrelated
since this occurs frequently on other files as well (bug 180334).

Comment 4

[Mats Palmgren (inactive)]

-> me - since I have a fix.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The problem is that we are using uninitialized stack variables which
are used as out-param for GetNewCString(...)  -  the problem is that this
function does not always assign the out-param and we are continuing to
nsMemory::Free(...) a random location. Here is GetNewCString:

nsresult
nsLoggingSink::GetNewCString(const nsAString& aValue, char** aResult)
{
  nsresult result=NS_OK;
  nsAutoString temp;
  result=QuoteText(aValue,temp);
  if(NS_SUCCEEDED(result)) {
    if(!temp.IsEmpty()) {
      *aResult = ToNewCString(temp);
    }
  }
  return result;
}


I have checked all uses of this function and there were two places that
did not initialize the out-param correctly (see patch).

On the other hand, since QuoteText(...) unconditionally returns NS_OK, what
has happened is that |temp.IsEmpty()| is true.	I find it a bit odd that we
expicitly avoid to assign |aResult| in this case.
Is this really correct?  Either way, the patch fixes the crash.

Comment 6

[harishd]

Comment on attachment 125478 [details] [diff] [review]
Patch rev. 1

r=harishd

Comment 7

[Brendan Eich [:brendan]]

GetNewCString is an accident waiting to happen still.  It should guarantee that
if it returns NS_OK, then it has also set its out parameter.

/be

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Agreed.  I now set it to |nsnull| when |temp.IsEmpty()|

Comment 9

[harishd]

Comment on attachment 125524 [details] [diff] [review]
Patch rev. 2

>   if(NS_SUCCEEDED(result)) {
>-    if(!temp.IsEmpty()) {
>-      *aResult = ToNewCString(temp);
>-    }
>+    *aResult = temp.IsEmpty() ? nsnull : ToNewCString(temp);

IMO, regardless of whether result returned is NS_OK or not we should still
initialize aResult.

That is, something like:

nsresult
nsLoggingSink::GetNewCString(const nsAString& aValue, char** aResult)
{
  NS_ENSURE_ARG_POINTER(aResult);
  *aResult = nsnull;
  nsAutoString temp;
  nsresult result = QuoteText(aValue, temp);
  if (NS_FAILED(result))
    return result;
  if (!temp.IsEmpty()) {
    *aResult = ToNewCString(temp);
    NS_ENSURE_TRUE(*aResult, NS_ERROR_OUT_OF_MEMORY);
  }
  return NS_OK;
}

Since this a debug only code I don't really care. So, r=harishd

Comment 10

[Mats Palmgren (inactive)]

I think "Patch rev. 2" is fine as it is.
Can someone check this in for me?  (I don't have cvs commit access.)

Comment 11

[Olivier Cahagne]

has this been checked in ?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated to tip

This is updated to trunk. I'm carrying forward harish's r= and requesting sr=.
I'll check it in once it has sr=. peterv, this should basically just be a
rubber-stamp sort of thing...

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fixed has been checked in.