Bug 136423 - [SOUP] Race condition when downloading a file due to the intermediate temporary file
Summary: [SOUP] Race condition when downloading a file due to the intermediate tempora...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 136372
  Show dependency treegraph
Reported: 2014-09-01 02:09 PDT by Carlos Garcia Campos
Modified: 2014-09-03 23:43 PDT (History)
2 users (show)

See Also:

Patch (6.84 KB, patch)
2014-09-01 11:49 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2014-09-01 13:11 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (6.68 KB, patch)
2014-09-03 12:54 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-09-01 02:09:53 PDT
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.
Comment 1 Michael Catanzaro 2014-09-01 11:49:38 PDT
Created attachment 237457 [details]
Comment 2 Michael Catanzaro 2014-09-01 13:11:47 PDT
Created attachment 237459 [details]
Comment 3 Carlos Garcia Campos 2014-09-02 01:24:46 PDT
Comment on attachment 237459 [details]

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
Comment 4 Michael Catanzaro 2014-09-03 12:54:21 PDT
Created attachment 237572 [details]
Comment 5 Carlos Garcia Campos 2014-09-03 23:07:07 PDT
Comment on attachment 237572 [details]

Perfect, thanks!
Comment 6 WebKit Commit Bot 2014-09-03 23:43:50 PDT
Comment on attachment 237572 [details]

Clearing flags on attachment: 237572

Committed r173252: <http://trac.webkit.org/changeset/173252>
Comment 7 WebKit Commit Bot 2014-09-03 23:43:53 PDT
All reviewed patches have been landed.  Closing bug.