Bug 78435 – UString.h will crash if is8bit is checked and UString::isNull() == true

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Adam Treat]

If you look at UString.h it seems that it is just a wrapper around WTF::StringImpl.  Most of the functions in UString.h check if it is null, but not UString::is8bit.  This can lead to a null pointer de-reference.  For instance, if you do this...

    m_mainFrame->script()->evaluateInWorld(ScriptSourceCode(""), ...);

Now, we shouldn't be doing his, but we also shouldn't crash because of it.

Adam

[Adam Treat]

Created attachment 126679 [details]
Fix

[Eric Seidel (no email)]

Comment on attachment 126679 [details]
Fix

How do we test this?

[Benjamin Poulain]

Comment on attachment 126679 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=126679&action=review

> Source/JavaScriptCore/runtime/UString.h:103
> -    bool is8Bit() const { return m_impl->is8Bit(); }
> +    bool is8Bit() const
> +    {
> +        return m_impl && m_impl->is8Bit();
> +    }

You are making UString inconsistent with WTFString. I think this is a really bad idea.

This was made by design.

[Benjamin Poulain]

(In reply to comment #0)
> If you look at UString.h it seems that it is just a wrapper around WTF::StringImpl.  Most of the functions in UString.h check if it is null, but not UString::is8bit.  This can lead to a null pointer de-reference.  For instance, if you do this...
> 
>     m_mainFrame->script()->evaluateInWorld(ScriptSourceCode(""), ...);

This looks similar to r106600

[Adam Treat]

(In reply to comment #3)
> (From update of attachment 126679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126679&action=review
> 
> > Source/JavaScriptCore/runtime/UString.h:103
> > -    bool is8Bit() const { return m_impl->is8Bit(); }
> > +    bool is8Bit() const
> > +    {
> > +        return m_impl && m_impl->is8Bit();
> > +    }
> 
> You are making UString inconsistent with WTFString. I think this is a really bad idea.
> 
> This was made by design.

How is this inconsistent exactly?? WTFString does not have an 'is8Bit' method, but StringImpl does... But I still don't get how this is inconsistent.

[Adam Treat]

(In reply to comment #4)
> (In reply to comment #0)
> > If you look at UString.h it seems that it is just a wrapper around WTF::StringImpl.  Most of the functions in UString.h check if it is null, but not UString::is8bit.  This can lead to a null pointer de-reference.  For instance, if you do this...
> > 
> >     m_mainFrame->script()->evaluateInWorld(ScriptSourceCode(""), ...);
> 
> This looks similar to r106600

Both fix the crash in the case of null source code, but this makes UString::is8Bit safe for UString::isNull() == true.

I don't get the inconsistent comment.  If anything, UString::is8Bit() is inconsistent with all the other methods in UString!

[Adam Treat]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #0)
> > > If you look at UString.h it seems that it is just a wrapper around WTF::StringImpl.  Most of the functions in UString.h check if it is null, but not UString::is8bit.  This can lead to a null pointer de-reference.  For instance, if you do this...
> > > 
> > >     m_mainFrame->script()->evaluateInWorld(ScriptSourceCode(""), ...);
> > 
> > This looks similar to r106600
> 
> Both fix the crash in the case of null source code, but this makes UString::is8Bit safe for UString::isNull() == true.
> 
> I don't get the inconsistent comment.  If anything, UString::is8Bit() is inconsistent with all the other methods in UString!

Or did you meant that StringImpl::is8Bit() will return true for a null string?

[Geoffrey Garen]

Adam,

I think Benjamin is suggesting -- and I agree -- that you should fix WTFString as well, so that UString and WTFString remain consistent. We need this consistency so we can eliminate UString over time, in favor of WTFString.

WTFString contains this:

    bool is8Bit() const { return m_impl->is8Bit(); }

You should fix it.

[Adam Treat]

(In reply to comment #8)
> Adam,
> 
> I think Benjamin is suggesting -- and I agree -- that you should fix WTFString as well, so that UString and WTFString remain consistent. We need this consistency so we can eliminate UString over time, in favor of WTFString.
> 
> WTFString contains this:
> 
>     bool is8Bit() const { return m_impl->is8Bit(); }
> 
> You should fix it.

Maybe I'(In reply to comment #8)
> Adam,
> 
> I think Benjamin is suggesting -- and I agree -- that you should fix WTFString as well, so that UString and WTFString remain consistent. We need this consistency so we can eliminate UString over time, in favor of WTFString.
> 
> WTFString contains this:
> 
>     bool is8Bit() const { return m_impl->is8Bit(); }
> 
> You should fix it.

Ok, I see the disconnect.  My tree (RIM's internal tree) is older and I don't have the change that added 'WTF::String::is8Bit()' yet so I didn't understand what Benjamin meant by it is inconsistent with WTF::String.  I will fix.

[Adam Treat]

(In reply to comment #9)
> (In reply to comment #8)
> > Adam,
> > 
> > I think Benjamin is suggesting -- and I agree -- that you should fix WTFString as well, so that UString and WTFString remain consistent. We need this consistency so we can eliminate UString over time, in favor of WTFString.
> > 
> > WTFString contains this:
> > 
> >     bool is8Bit() const { return m_impl->is8Bit(); }
> > 
> > You should fix it.
> 
> Maybe I'(In reply to comment #8)
> > Adam,
> > 
> > I think Benjamin is suggesting -- and I agree -- that you should fix WTFString as well, so that UString and WTFString remain consistent. We need this consistency so we can eliminate UString over time, in favor of WTFString.
> > 
> > WTFString contains this:
> > 
> >     bool is8Bit() const { return m_impl->is8Bit(); }
> > 
> > You should fix it.
> 
> Ok, I see the disconnect.  My tree (RIM's internal tree) is older and I don't have the change that added 'WTF::String::is8Bit()' yet so I didn't understand what Benjamin meant by it is inconsistent with WTF::String.  I will fix.


BTW, we are only a few months behind of TOT and we're trying to get upstream as fast as possible so we can work in TOT and these misunderstandings will go away.  Sorry for the disconnect.  Should have checked TOT I guess.

[Alexey Proskuryakov]

In bug 77593, we didn't add a null check because it's not there by design, for better performance. Sadly, this discussion was not captured in the bug, it was on IRC.

If we decide to make this change after all, the fix for bug 77593 should likely be reverted, and a similar cheek in ScriptController should be removed, too.

[Adam Treat]

(In reply to comment #11)
> In bug 77593, we didn't add a null check because it's not there by design, for better performance. Sadly, this discussion was not captured in the bug, it was on IRC.
> 
> If we decide to make this change after all, the fix for bug 77593 should likely be reverted, and a similar cheek in ScriptController should be removed, too.

Interesting.  Well, I can say that my patch doesn't fix the crash as there are other places in the code that do a similar null-pointer de-reference if you get further than just returning JSUndefined where Benjamin did.  Let me know if I should continue with this or if you really intend to let  UString::is8Bit and/or WTF::String::is8Bit have potential null pointer derefs like this.  I think at the least that an ASSERT should be added which won't impact release build performance since it is defined out.

[Geoffrey Garen]

> In bug 77593, we didn't add a null check because it's not there by design, for better performance. Sadly, this discussion was not captured in the bug, it was on IRC.

I wasn't privy to that discussion, but considering performance, I still think the null check is the right move.

All C++ code that checks is8Bit() goes on to dereference the string, which checks for NULL. I'd expect the standard control-flow analysis in any optimizing compiler to merge the two NULL checks, resulting in no runtime performance cost.

> If we decide to make this change after all, the fix for bug 77593 should likely be reverted, and a similar cheek in ScriptController should be removed, too.

Sounds good.

[Benjamin Poulain]

(In reply to comment #13)
> > In bug 77593, we didn't add a null check because it's not there by design, for better performance. Sadly, this discussion was not captured in the bug, it was on IRC.
> 
> I wasn't privy to that discussion, but considering performance, I still think the null check is the right move.
> 
> All C++ code that checks is8Bit() goes on to dereference the string, which checks for NULL. I'd expect the standard control-flow analysis in any optimizing compiler to merge the two NULL checks, resulting in no runtime performance cost.

In my opinion, either is8Bit() should have the null check, or characters8() and characters16() should have their null check removed. 

The problem was not so much against adding the null check, but about making sure this does not causes any regression.
This has impact on the whole codebase. I had no time to ensure no regressions are introduced, so we fixed the bug the same way this problem is solved everywhere else.

[Adam Treat]

Comment on attachment 126679 [details]
Fix

Obsolete patch.

[Alexey Proskuryakov]

What's the verdict here? Can the bug be closed?

[Benjamin Poulain]

> What's the verdict here? Can the bug be closed?

Yep, none of that code exist anymore.