<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[173252] trunk/Source/WebKit2</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/173252">173252</a></dd>
<dt>Author</dt> <dd>commit-queue@webkit.org</dd>
<dt>Date</dt> <dd>2014-09-03 23:43:35 -0700 (Wed, 03 Sep 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>[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 <mcatanzaro@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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebKit2ChangeLog">trunk/Source/WebKit2/ChangeLog</a></li>
<li><a href="#trunkSourceWebKit2SharedDownloadssoupDownloadSoupcpp">trunk/Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebKit2ChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/ChangeLog (173251 => 173252)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,25 @@
</span><ins>+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.
+
</ins><span class="cx"> 2014-09-03 David Kilzer <ddkilzer@apple.com>
</span><span class="cx">
</span><span class="cx"> JavaScriptCore should build with newer clang
</span></span></pre></div>
<a id="trunkSourceWebKit2SharedDownloadssoupDownloadSoupcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp (173251 => 173252)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -58,16 +58,20 @@
</span><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><del>- void deleteIntermediateFileInNeeded()
</del><ins>+ void deleteFilesIfNeeded()
</ins><span class="cx"> {
</span><del>- if (!m_intermediateFile)
- return;
- g_file_delete(m_intermediateFile.get(), nullptr, nullptr);
</del><ins>+ 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);
+ }
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void downloadFailed(const ResourceError& error)
</span><span class="cx"> {
</span><del>- deleteIntermediateFileInNeeded();
</del><ins>+ deleteFilesIfNeeded();
</ins><span class="cx"> m_download->didFail(error, IPC::DataReference());
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -89,8 +93,8 @@
</span><span class="cx"> suggestedFilename = decodeURLEscapeSequences(url.lastPathComponent());
</span><span class="cx"> }
</span><span class="cx">
</span><del>- m_destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, m_allowOverwrite);
- if (m_destinationURI.isEmpty()) {
</del><ins>+ String destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, m_allowOverwrite);
+ if (destinationURI.isEmpty()) {
</ins><span class="cx"> #if PLATFORM(GTK)
</span><span class="cx"> GUniquePtr<char> buffer(g_strdup_printf(_("Cannot determine destination URI for download with suggested filename %s"), suggestedFilename.utf8().data()));
</span><span class="cx"> String errorMessage = String::fromUTF8(buffer.get());
</span><span class="lines">@@ -101,16 +105,28 @@
</span><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- String intermediateURI = m_destinationURI + ".wkdownload";
</del><ins>+ 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";
</ins><span class="cx"> m_intermediateFile = adoptGRef(g_file_new_for_uri(intermediateURI.utf8().data()));
</span><del>- GUniqueOutPtr<GError> error;
</del><span class="cx"> m_outputStream = adoptGRef(g_file_replace(m_intermediateFile.get(), 0, TRUE, G_FILE_CREATE_NONE, 0, &error.outPtr()));
</span><span class="cx"> if (!m_outputStream) {
</span><span class="cx"> downloadFailed(platformDownloadDestinationError(response, error->message));
</span><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- m_download->didCreateDestination(m_destinationURI);
</del><ins>+ m_download->didCreateDestination(destinationURI);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void didReceiveData(ResourceHandle*, const char* data, unsigned length, int /*encodedDataLength*/)
</span><span class="lines">@@ -134,10 +150,10 @@
</span><span class="cx"> {
</span><span class="cx"> m_outputStream = 0;
</span><span class="cx">
</span><ins>+ ASSERT(m_destinationFile);
</ins><span class="cx"> ASSERT(m_intermediateFile);
</span><del>- GRefPtr<GFile> destinationFile = adoptGRef(g_file_new_for_uri(m_destinationURI.utf8().data()));
</del><span class="cx"> GUniqueOutPtr<GError> error;
</span><del>- if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), m_allowOverwrite ? G_FILE_COPY_OVERWRITE : G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) {
</del><ins>+ if (!g_file_move(m_intermediateFile.get(), m_destinationFile.get(), G_FILE_COPY_OVERWRITE, nullptr, nullptr, nullptr, &error.outPtr())) {
</ins><span class="cx"> downloadFailed(platformDownloadDestinationError(m_response, error->message));
</span><span class="cx"> return;
</span><span class="cx"> }
</span><span class="lines">@@ -146,7 +162,7 @@
</span><span class="cx"> CString uri = m_response.url().string().utf8();
</span><span class="cx"> g_file_info_set_attribute_string(info.get(), "metadata::download-uri", uri.data());
</span><span class="cx"> g_file_info_set_attribute_string(info.get(), "xattr::xdg.origin.url", uri.data());
</span><del>- g_file_set_attributes_async(destinationFile.get(), info.get(), G_FILE_QUERY_INFO_NONE, G_PRIORITY_DEFAULT, nullptr, nullptr, nullptr);
</del><ins>+ g_file_set_attributes_async(m_destinationFile.get(), info.get(), G_FILE_QUERY_INFO_NONE, G_PRIORITY_DEFAULT, nullptr, nullptr, nullptr);
</ins><span class="cx">
</span><span class="cx"> m_download->didFinish();
</span><span class="cx"> }
</span><span class="lines">@@ -169,7 +185,7 @@
</span><span class="cx"> void cancel(ResourceHandle* handle)
</span><span class="cx"> {
</span><span class="cx"> handle->cancel();
</span><del>- deleteIntermediateFileInNeeded();
</del><ins>+ deleteFilesIfNeeded();
</ins><span class="cx"> m_download->didCancel(IPC::DataReference());
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -193,7 +209,7 @@
</span><span class="cx"> Download* m_download;
</span><span class="cx"> GRefPtr<GFileOutputStream> m_outputStream;
</span><span class="cx"> ResourceResponse m_response;
</span><del>- String m_destinationURI;
</del><ins>+ GRefPtr<GFile> m_destinationFile;
</ins><span class="cx"> GRefPtr<GFile> m_intermediateFile;
</span><span class="cx"> ResourceResponse m_delayedResponse;
</span><span class="cx"> GMainLoopSource m_handleResponseLater;
</span></span></pre>
</div>
</div>
</body>
</html>