[SOUP] Race condition when downloading a file due to the intermediate temporary file

[Carlos Garcia Campos]

When we start a new download, we don't create the actual destination file, but a temporary file to be atomically copied to the final destination once the download is completed. The user or any other application might create a file with the same path and name than the destination, that we might overwrite when the download finishes. We need to make sure we create the final destination as soon as it's known, so that other applications (or instances) that checks if the destination exists will see the file. So, I think we should create first the actual destination, failing if it already exists and allowOverwrite is false. If it doesn't fail then we create the temporary and continue the download. Once it finishes, we move the temporary to the final destination using G_FILE_COPY_OVERWRITE unconditionally, since at this point we should have already failed if allowOverwrite was false. In case of failure or cancellation we should probably delete both files, not only the intermediate one.

[Michael Catanzaro]

Created attachment 237457 [details]
Patch

[Michael Catanzaro]

Created attachment 237459 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 237459 [details]
Patch

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

Looks good to me in general, I have just a few comments

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:67
> +        if (m_createdDestination) {
> +            ASSERT(m_destinationFile);
> +            g_file_delete(m_destinationFile.get(), nullptr, nullptr);
> +        }

I think we can use m_destinationFile to know if the destination was successfully created.

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:115
> +            outputStream = adoptGRef(g_file_replace(m_destinationFile.get(), nullptr, false, G_FILE_CREATE_NONE, nullptr, &error.outPtr()));

g_file_replace expects a gboolean, so better use FALSE, instead of false, even though it doesn't really matter.

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:121
> +        if (!outputStream) {
> +            downloadFailed(platformDownloadDestinationError(response, error->message));
> +            return;
> +        }

And here you could m_destinationFile.reset() and you don't need the m_createdDestination bool member

[Michael Catanzaro]

Created attachment 237572 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 237572 [details]
Patch

Perfect, thanks!

[WebKit Commit Bot]

Comment on attachment 237572 [details]
Patch

Clearing flags on attachment: 237572

Committed r173252: <http://trac.webkit.org/changeset/173252>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.