Bug 27494 – Crash/Unexpected behavior if call ScriptSourceCode::source on a CachedScript

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Daniel Bates]

ScriptSourceCode::source assumes the provider for the source code of a script is of type StringSourceProvider. However, if the source code is from a cached script, then the provider is CachedScriptSourceProvider. Hence, unexpected behavior and/or a crash can follow if you call ScriptSourceCode::source on a cached script.

[Daniel Bates]

You can reproducibly cause a crash by adding the following line above line 87 in ScriptController.cpp (http://trac.webkit.org/browser/trunk/WebCore/bindings/js/ScriptController.cpp#L87):

printf("%s\n", sourceCode.source().ascii().data());

Then run layout test /http/tests/security/xssAuditor/base-href.html

[Daniel Bates]

Created attachment 33165 [details]
Patch

Created a new base class called ScriptSourceProvider which is inherited by both StringSourceProvider and CachedScriptSourceProvider.

Blindly edited the WebCore Visual C++ project file.

[Adam Barth]

Comment on attachment 33165 [details]
Patch

The code looks good, but when you add a file to the project, you should really add it to all the build systems.  I think you've missed WebCore.gypi for example.   You should find "ScriptSourceCode.h" and make sure your new file shows up in all the same places.

[Daniel Bates]

Created attachment 33230 [details]
Patch

Added ScriptSourceProvider to WebCore.gypi.

[Adam Barth]

Comment on attachment 33230 [details]
Patch

There are yet more build systems in the tree.  Yes it sucks.  For example:

http://trac.webkit.org/browser/trunk/WebCore/WebCore.pro

Also, can we test for this crash in a LayoutTest?  If it involves a cached script, we might need to load the same script twice.

[Daniel Bates]

Created attachment 33239 [details]
Patch

Added bindings/js/ScriptSourceProvider.h, ScriptSourceCode.h, StringSourceProvider.h to WebCore.pro.

I hope I've got all the build systems covered.

[Daniel Bates]

It does not appear that anybody (besides ScriptController.cpp) calls ScriptSourceCode::source. So, I don't believe we can test this crash in a LayoutTest alone.

(In reply to comment #5)
> (From update of attachment 33230 [details])
> There are yet more build systems in the tree.  Yes it sucks.  For example:
> 
> http://trac.webkit.org/browser/trunk/WebCore/WebCore.pro
> 
> Also, can we test for this crash in a LayoutTest?  If it involves a cached
> script, we might need to load the same script twice.

[Adam Barth]

Comment on attachment 33239 [details]
Patch

Confirmed with Dan on IRC that this crash only surfaces after he makes another change he's working on.  It's a crash fix from the future!

[Daniel Bates]

Created attachment 33245 [details]
Patch

I hope this fixes the Xcode issue.

[Daniel Bates]

Created attachment 33246 [details]
Patch

Oops. Forgot the change log last time.

[Adam Barth]

Comment on attachment 33246 [details]
Patch

Ok.  Let's try it.

[Adam Barth]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.pro
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/bindings/js/CachedScriptSourceProvider.h
	M	WebCore/bindings/js/ScriptSourceCode.h
	A	WebCore/bindings/js/ScriptSourceProvider.h
	M	WebCore/bindings/js/StringSourceProvider.h
Committed r46216
	M	WebCore/WebCore.pro
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/bindings/js/StringSourceProvider.h
	M	WebCore/bindings/js/CachedScriptSourceProvider.h
	M	WebCore/bindings/js/ScriptSourceCode.h
	A	WebCore/bindings/js/ScriptSourceProvider.h
	M	WebCore/WebCore.xcodeproj/project.pbxproj
r46216 = 2e428bb4856f165dba70c85ccd5666e79af5d6bf (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46216

[Alexey Proskuryakov]

+        virtual ~ScriptSourceProvider() { }

There is no need to explicitly define an empty inline destructor here - the compiler would generate an identical one. It could make sense to define it as non-inline though, to reduce code bloat.