[webkit-changes] [173252] trunk/Source/WebKit2

Wed, 03 Sep 2014 23:44:01 -0700

Title: [173252] trunk/Source/WebKit2
Revision
173252
Author
commit-qu...@webkit.org
Date
2014-09-03 23:43:35 -0700 (Wed, 03 Sep 2014)

Log Message

[SOUP] Race condition when downloading a file due to the intermediate temporary file
https://bugs.webkit.org/show_bug.cgi?id=136423

Patch by Michael Catanzaro <mcatanz...@igalia.com> on 2014-09-03
Reviewed by Carlos Garcia Campos.

* Shared/Downloads/soup/DownloadSoup.cpp:
(WebKit::DownloadClient::DownloadClient): Replace m_destinationURI with
m_destinationFile and add m_createdDestination.
(WebKit::DownloadClient::deleteFilesIfNeeded): Added.
(WebKit::DownloadClient::downloadFailed): Call deleteFilesIfNeeded.
(WebKit::DownloadClient::didReceiveResponse): Attempt to create the
destination file before the intermediate file. Fail here if the file
exists and overwrite is not allowed, so we don't erroneously fire the
didCreateDestination event or waste time downloading the file when we
know the download will fail.
(WebKit::DownloadClient::didFinishLoading): Unconditionally overwrite
the empty destination file.
(WebKit::DownloadClient::cancel): Call deleteFilesIfNeeded.
(WebKit::DownloadClient::deleteIntermediateFileInNeeded): Deleted.

Modified Paths

Diff

Modified: trunk/Source/WebKit2/ChangeLog (173251 => 173252)


--- trunk/Source/WebKit2/ChangeLog	2014-09-04 05:53:43 UTC (rev 173251)
+++ trunk/Source/WebKit2/ChangeLog	2014-09-04 06:43:35 UTC (rev 173252)
@@ -1,3 +1,25 @@
+2014-09-03  Michael Catanzaro  <mcatanz...@igalia.com>
+
+        [SOUP] Race condition when downloading a file due to the intermediate temporary file
+        https://bugs.webkit.org/show_bug.cgi?id=136423
+
+        Reviewed by Carlos Garcia Campos.
+
+        * Shared/Downloads/soup/DownloadSoup.cpp:
+        (WebKit::DownloadClient::DownloadClient): Replace m_destinationURI with
+        m_destinationFile and add m_createdDestination.
+        (WebKit::DownloadClient::deleteFilesIfNeeded): Added.
+        (WebKit::DownloadClient::downloadFailed): Call deleteFilesIfNeeded.
+        (WebKit::DownloadClient::didReceiveResponse): Attempt to create the
+        destination file before the intermediate file. Fail here if the file
+        exists and overwrite is not allowed, so we don't erroneously fire the
+        didCreateDestination event or waste time downloading the file when we
+        know the download will fail.
+        (WebKit::DownloadClient::didFinishLoading): Unconditionally overwrite
+        the empty destination file.
+        (WebKit::DownloadClient::cancel): Call deleteFilesIfNeeded.
+        (WebKit::DownloadClient::deleteIntermediateFileInNeeded): Deleted.
+
 2014-09-03  David Kilzer  <ddkil...@apple.com>
 
         _javascript_Core should build with newer clang

Modified: trunk/Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp (173251 => 173252)


--- trunk/Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp	2014-09-04 05:53:43 UTC (rev 173251)
+++ trunk/Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp	2014-09-04 06:43:35 UTC (rev 173252)
@@ -58,16 +58,20 @@
     {
     }
 
-    void deleteIntermediateFileInNeeded()
+    void deleteFilesIfNeeded()
     {
-        if (!m_intermediateFile)
-            return;
-        g_file_delete(m_intermediateFile.get(), nullptr, nullptr);
+        if (m_destinationFile)
+            g_file_delete(m_destinationFile.get(), nullptr, nullptr);
+
+        if (m_intermediateFile) {
+            ASSERT(m_destinationFile);
+            g_file_delete(m_intermediateFile.get(), nullptr, nullptr);
+        }
     }
 
     void downloadFailed(const ResourceError& error)
     {
-        deleteIntermediateFileInNeeded();
+        deleteFilesIfNeeded();
         m_download->didFail(error, IPC::DataReference());
     }
 
@@ -89,8 +93,8 @@
             suggestedFilename = decodeURLEscapeSequences(url.lastPathComponent());
         }
 
-        m_destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, m_allowOverwrite);
-        if (m_destinationURI.isEmpty()) {
+        String destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, m_allowOverwrite);
+        if (destinationURI.isEmpty()) {
 #if PLATFORM(GTK)
             GUniquePtr<char> buffer(g_strdup_printf(_("Cannot determine destination URI for download with suggested filename %s"), suggestedFilename.utf8().data()));
             String errorMessage = String::fromUTF8(buffer.get());
@@ -101,16 +105,28 @@
             return;
         }
 
-        String intermediateURI = m_destinationURI + ".wkdownload";
+        m_destinationFile = adoptGRef(g_file_new_for_uri(destinationURI.utf8().data()));
+        GRefPtr<GFileOutputStream> outputStream;
+        GUniqueOutPtr<GError> error;
+        if (m_allowOverwrite)
+            outputStream = adoptGRef(g_file_replace(m_destinationFile.get(), nullptr, FALSE, G_FILE_CREATE_NONE, nullptr, &error.outPtr()));
+        else
+            outputStream = adoptGRef(g_file_create(m_destinationFile.get(), G_FILE_CREATE_NONE, nullptr, &error.outPtr()));
+        if (!outputStream) {
+            m_destinationFile.clear();
+            downloadFailed(platformDownloadDestinationError(response, error->message));
+            return;
+        }
+
+        String intermediateURI = destinationURI + ".wkdownload";
         m_intermediateFile = adoptGRef(g_file_new_for_uri(intermediateURI.utf8().data()));
-        GUniqueOutPtr<GError> error;
         m_outputStream = adoptGRef(g_file_replace(m_intermediateFile.get(), 0, TRUE, G_FILE_CREATE_NONE, 0, &error.outPtr()));
         if (!m_outputStream) {
             downloadFailed(platformDownloadDestinationError(response, error->message));
             return;
         }
 
-        m_download->didCreateDestination(m_destinationURI);
+        m_download->didCreateDestination(destinationURI);
     }
 
     void didReceiveData(ResourceHandle*, const char* data, unsigned length, int /*encodedDataLength*/)
@@ -134,10 +150,10 @@
     {
         m_outputStream = 0;
 
+        ASSERT(m_destinationFile);
         ASSERT(m_intermediateFile);
-        GRefPtr<GFile> destinationFile = adoptGRef(g_file_new_for_uri(m_destinationURI.utf8().data()));
         GUniqueOutPtr<GError> error;
-        if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), m_allowOverwrite ? G_FILE_COPY_OVERWRITE : G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) {
+        if (!g_file_move(m_intermediateFile.get(), m_destinationFile.get(), G_FILE_COPY_OVERWRITE, nullptr, nullptr, nullptr, &error.outPtr())) {
             downloadFailed(platformDownloadDestinationError(m_response, error->message));
             return;
         }
@@ -146,7 +162,7 @@
         CString uri = m_response.url().string().utf8();
         g_file_info_set_attribute_string(info.get(), "metadata::download-uri", uri.data());
         g_file_info_set_attribute_string(info.get(), "xattr::xdg.origin.url", uri.data());
-        g_file_set_attributes_async(destinationFile.get(), info.get(), G_FILE_QUERY_INFO_NONE, G_PRIORITY_DEFAULT, nullptr, nullptr, nullptr);
+        g_file_set_attributes_async(m_destinationFile.get(), info.get(), G_FILE_QUERY_INFO_NONE, G_PRIORITY_DEFAULT, nullptr, nullptr, nullptr);
 
         m_download->didFinish();
     }
@@ -169,7 +185,7 @@
     void cancel(ResourceHandle* handle)
     {
         handle->cancel();
-        deleteIntermediateFileInNeeded();
+        deleteFilesIfNeeded();
         m_download->didCancel(IPC::DataReference());
     }
 
@@ -193,7 +209,7 @@
     Download* m_download;
     GRefPtr<GFileOutputStream> m_outputStream;
     ResourceResponse m_response;
-    String m_destinationURI;
+    GRefPtr<GFile> m_destinationFile;
     GRefPtr<GFile> m_intermediateFile;
     ResourceResponse m_delayedResponse;
     GMainLoopSource m_handleResponseLater;

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to