Changeset 173252 in webkit

Timestamp:

Sep 3, 2014 11:43:35 PM (10 years ago)

Author:

commit-queue@webkit.org

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 <Michael Catanzaro> 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.

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• Shared/Downloads/soup/DownloadSoup.cpp (modified) (8 diffs)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2014-09-03  Michael Catanzaro  <mcatanzaro@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  <ddkilzer@apple.com>
 

trunk/Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp

@@ -59 +59 @@
     }
 
-    void deleteIntermediateFileInNeeded()
-    {
-        if (!m_intermediateFile)
-            return;
-        g_file_delete(m_intermediateFile.get(), nullptr, nullptr);
+    void deleteFilesIfNeeded()
+    {
+        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());
     }
@@ -90 +94 @@
         }
 
-        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()));
@@ -102 +106 @@
         }
 
-        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) {
@@ -111 +127 @@
         }
 
-        m_download->didCreateDestination(m_destinationURI);
+        m_download->didCreateDestination(destinationURI);
     }
 
@@ -135 +151 @@
         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;
@@ -147 +163 @@
         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();
@@ -170 +186 @@
     {
         handle->cancel();
-        deleteIntermediateFileInNeeded();
+        deleteFilesIfNeeded();
         m_download->didCancel(IPC::DataReference());
     }
@@ -194 +210 @@
     GRefPtr<GFileOutputStream> m_outputStream;
     ResourceResponse m_response;
-    String m_destinationURI;
+    GRefPtr<GFile> m_destinationFile;
     GRefPtr<GFile> m_intermediateFile;
     ResourceResponse m_delayedResponse;