Bug 49539 – window.requestFileSystem(16) NULL ptr

• Component: DOM
• Status: RESOLVED FIXED

[Berend-Jan Wever]

Created attachment 73889 [details]
Repro

Repro:
<script>window.requestFileSystem(16)</script>

id:             chrome.dll!WebCore::DOMFileSystem::DispatchCallbackTask<...>::performTask ReadAV@NULL (2ef0152de44115d5e3f1de3111aba5dd)
description:    Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::DOMFileSystem::DispatchCallbackTask<...>::performTask
stack:          chrome.dll!WebCore::DOMFileSystem::DispatchCallbackTask<...>::performTask
                chrome.dll!WebCore::performTask
                chrome.dll!RunnableFunction<void 
                chrome.dll!MessageLoop::RunTask
                chrome.dll!MessageLoop::DoWork
                chrome.dll!base::MessagePumpDefault::Run
                chrome.dll!MessageLoop::RunInternal
                chrome.dll!MessageLoop::Run
                chrome.dll!RendererMain
                chrome.dll!ChromeMain
                chrome.exe!MainDllLoader::Launch
                chrome.exe!wWinMain
                chrome.exe!__tmainCRTStartup
                ...

[Berend-Jan Wever]

Code is not checking if errorCallback is NULL before scheduling a callback on error. Suggested patch:

void DOMWindow::requestFileSystem(int type, long long size, PassRefPtr<FileSystemCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback)
{
    Document* document = this->document();
    if (!document)
        return;

    if (!AsyncFileSystem::isAvailable() || !document->securityOrigin()->canAccessFileSystem()) {
-       DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::SECURITY_ERR));
+       if (errorCallback)
+           DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::SECURITY_ERR));
        return;
    }

    AsyncFileSystem::Type fileSystemType = static_cast<AsyncFileSystem::Type>(type);
    if (fileSystemType != AsyncFileSystem::Temporary && fileSystemType != AsyncFileSystem::Persistent) {
-       DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::INVALID_MODIFICATION_ERR));
+       if (errorCallback)
+           DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::INVALID_MODIFICATION_ERR));
        return;
    }

    LocalFileSystem::localFileSystem().requestFileSystem(document, fileSystemType, size, FileSystemCallbacks::create(successCallback, errorCallback, document), false);
}

[Berend-Jan Wever]

I'm implementing a patch

[Berend-Jan Wever]

Created attachment 84761 [details]
Patch

[WebKit Review Bot]

Attachment 84761 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/DOMWindow.cpp:746:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/DOMWindow.cpp:753:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Berend-Jan Wever]

Created attachment 84763 [details]
Patch

[Berend-Jan Wever]

Variation:
<script>setTimeout(resolveLocalFileSystemURI,1)</script>
I'll create a patch for both issues

[Berend-Jan Wever]

Created attachment 85146 [details]
Patch for both requestFileSystem and resolveLocalFileSystemURI

[Berend-Jan Wever]

Adam/Kinuko: you seem to have implemented/reviewed these functions originally - any chance you can review this patch?

[Eric Seidel (no email)]

Comment on attachment 85146 [details]
Patch for both requestFileSystem and resolveLocalFileSystemURI

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

> Source/WebCore/page/DOMWindow.cpp:746
> +        if (errorCallback)
> +            DOMFileSystem::scheduleCallback(document, errorCallback, FileError::create(FileError::SECURITY_ERR));

Random drive-by nit:  Seems it might be time to think about making a helper function for this.  Something like:

scheduleFileCallback(document, errorCallback, SECURITY_ERR);

which includes the if (!errorCallback) return, and the FileError creation.

It may not feel worth it.  Unsure.

[Eric Seidel (no email)]

The patch does not appear marked for review (unless I cleared it accidentally?)

[Kinuko Yasuda]

Thanks for fixing it.
I think we could just remove the ASSERT(callback) in DOMFileSystem::scheduleCallback and schedule a task iff callback is given and not null.  Otherwise it seems we have to put such an if condition whenever we call it.

diff --git a/Source/WebCore/fileapi/DOMFileSystem.h b/Source/WebCore/fileapi/DOMFileSystem.h
index 1d820f1..38a054a 100644
--- a/Source/WebCore/fileapi/DOMFileSystem.h
+++ b/Source/WebCore/fileapi/DOMFileSystem.h
@@ -101,8 +101,8 @@ template <typename CB, typename CBArg>
 void DOMFileSystem::scheduleCallback(ScriptExecutionContext* scriptExecutionContext, PassRefPtr<CB> callback, PassRefPtr<CBArg> arg)
 {
     ASSERT(scriptExecutionContext->isContextThread());
-    ASSERT(callback);
-    scriptExecutionContext->postTask(new DispatchCallbackTask<CB, CBArg>(callback, arg));
+    if (callback)
+        scriptExecutionContext->postTask(new DispatchCallbackTask<CB, CBArg>(callback, arg));
 }
 
 } // namespace

[Berend-Jan Wever]

Created attachment 85462 [details]
Patch for DOMFileSystem::scheduleCallback

[Kinuko Yasuda]

Comment on attachment 85462 [details]
Patch for DOMFileSystem::scheduleCallback

Thanks, this patch looks good to me (though I'm not a reviewer).

[Kent Tamura]

Comment on attachment 85462 [details]
Patch for DOMFileSystem::scheduleCallback

The patch doesn't have filesystem-no-callback-null-ptr-crash-expected.txt.

[Berend-Jan Wever]

Created attachment 95445 [details]
Patch

Same patch, but with fixed layout test and added "-expected.txt" file.

[Berend-Jan Wever]

Created attachment 95446 [details]
Same patch, but updated changelog date

[Berend-Jan Wever]

@Kent Tamura: can you review+/commit+?

[WebKit Commit Bot]

Comment on attachment 95446 [details]
Same patch, but updated changelog date

Clearing flags on attachment: 95446

Committed r87758: <http://trac.webkit.org/changeset/87758>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Berend-Jan Wever]

Thanks!
Chromium: https://code.google.com/p/chromium/issues/detail?id=63204

[Ademar Reis]

Revision r87758 cherry-picked into qtwebkit-2.2 with commit f1148b9 <http://gitorious.org/webkit/qtwebkit/commit/f1148b9>

[Berend-Jan Wever]

bug 61936 was opened because the test is flaky and I don't know why - can somebody help me find out?