Bug 49539 - window.requestFileSystem(16) NULL ptr
Summary: window.requestFileSystem(16) NULL ptr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Berend-Jan Wever
URL: chrome.dll!WebCore::DOMFileSystem::Di...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-15 06:54 PST by Berend-Jan Wever
Modified: 2011-06-02 12:52 PDT (History)
8 users (show)

See Also:


Attachments
Repro (45 bytes, text/html)
2010-11-15 06:54 PST, Berend-Jan Wever
no flags Details
Patch (3.28 KB, patch)
2011-03-04 09:33 PST, Berend-Jan Wever
no flags Details | Formatted Diff | Diff
Patch (3.30 KB, patch)
2011-03-04 09:39 PST, Berend-Jan Wever
skylined: review-
Details | Formatted Diff | Diff
Patch for both requestFileSystem and resolveLocalFileSystemURI (4.52 KB, patch)
2011-03-09 01:54 PST, Berend-Jan Wever
no flags Details | Formatted Diff | Diff
Patch for DOMFileSystem::scheduleCallback (2.98 KB, patch)
2011-03-11 05:38 PST, Berend-Jan Wever
tkent: review-
Details | Formatted Diff | Diff
Patch (3.46 KB, patch)
2011-05-31 09:46 PDT, Berend-Jan Wever
no flags Details | Formatted Diff | Diff
Same patch, but updated changelog date (3.46 KB, patch)
2011-05-31 09:49 PDT, Berend-Jan Wever
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2010-11-15 06:54:41 PST
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
                ...
Comment 1 Berend-Jan Wever 2011-01-26 05:12:22 PST
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);
}
Comment 2 Berend-Jan Wever 2011-03-01 08:05:55 PST
I'm implementing a patch
Comment 3 Berend-Jan Wever 2011-03-04 09:33:56 PST
Created attachment 84761 [details]
Patch
Comment 4 WebKit Review Bot 2011-03-04 09:37:21 PST
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.
Comment 5 Berend-Jan Wever 2011-03-04 09:39:51 PST
Created attachment 84763 [details]
Patch
Comment 6 Berend-Jan Wever 2011-03-09 01:38:11 PST
Variation:
<script>setTimeout(resolveLocalFileSystemURI,1)</script>
I'll create a patch for both issues
Comment 7 Berend-Jan Wever 2011-03-09 01:54:59 PST
Created attachment 85146 [details]
Patch for both requestFileSystem and resolveLocalFileSystemURI
Comment 8 Berend-Jan Wever 2011-03-09 02:04:35 PST
Adam/Kinuko: you seem to have implemented/reviewed these functions originally - any chance you can review this patch?
Comment 9 Eric Seidel (no email) 2011-03-09 09:07:03 PST
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.
Comment 10 Eric Seidel (no email) 2011-03-09 09:07:30 PST
The patch does not appear marked for review (unless I cleared it accidentally?)
Comment 11 Kinuko Yasuda 2011-03-09 13:03:21 PST
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
Comment 12 Berend-Jan Wever 2011-03-11 05:38:27 PST
Created attachment 85462 [details]
Patch for DOMFileSystem::scheduleCallback
Comment 13 Kinuko Yasuda 2011-04-04 21:29:15 PDT
Comment on attachment 85462 [details]
Patch for DOMFileSystem::scheduleCallback

Thanks, this patch looks good to me (though I'm not a reviewer).
Comment 14 Kent Tamura 2011-04-04 22:06:41 PDT
Comment on attachment 85462 [details]
Patch for DOMFileSystem::scheduleCallback

The patch doesn't have filesystem-no-callback-null-ptr-crash-expected.txt.
Comment 15 Berend-Jan Wever 2011-05-31 09:46:30 PDT
Created attachment 95445 [details]
Patch

Same patch, but with fixed layout test and added "-expected.txt" file.
Comment 16 Berend-Jan Wever 2011-05-31 09:49:01 PDT
Created attachment 95446 [details]
Same patch, but updated changelog date
Comment 17 Berend-Jan Wever 2011-05-31 09:50:13 PDT
@Kent Tamura: can you review+/commit+?
Comment 18 WebKit Commit Bot 2011-05-31 15:40:32 PDT
Comment on attachment 95446 [details]
Same patch, but updated changelog date

Clearing flags on attachment: 95446

Committed r87758: <http://trac.webkit.org/changeset/87758>
Comment 19 WebKit Commit Bot 2011-05-31 15:40:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Berend-Jan Wever 2011-06-01 05:01:45 PDT
Thanks!
Chromium: https://code.google.com/p/chromium/issues/detail?id=63204
Comment 21 Ademar Reis 2011-06-01 12:32:01 PDT
Revision r87758 cherry-picked into qtwebkit-2.2 with commit f1148b9 <http://gitorious.org/webkit/qtwebkit/commit/f1148b9>
Comment 22 Berend-Jan Wever 2011-06-02 12:52:01 PDT
bug 61936 was opened because the test is flaky and I don't know why - can somebody help me find out?