Fix races when the same bits are downloaded from 2 URLs.

shell/application_manager/network_fetcher.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "shell/application_manager/network_fetcher.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/process/process.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "crypto/secure_hash.h"
 #include "crypto/sha2.h"
 #include "mojo/common/common_type_converters.h"
 #include "mojo/common/data_pipe_utils.h"
 #include "mojo/services/network/public/interfaces/network_service.mojom.h"
 #include "shell/application_manager/data_pipe_peek.h"
+#include "shell/switches.h"
 
 namespace mojo {
 namespace shell {
 
 NetworkFetcher::NetworkFetcher(bool disable_cache,
                                const GURL& url,
                                NetworkService* network_service,
                                const FetchCallback& loader_callback)
     : Fetcher(loader_callback),
       disable_cache_(false),
@@ skipping 76 matching lines @@
   }
   // The output is really a vector of unit8, we're cheating by using a string.
   std::string output(crypto::kSHA256Length, 0);
   ctx->Finish(string_as_array(&output), output.size());
   output = base::HexEncode(output.c_str(), output.size());
   // Using lowercase for compatiblity with sha256sum output.
   *digest_string = base::StringToLowerASCII(output);
   return true;
 }
 
-bool NetworkFetcher::RenameToAppId(const base::FilePath& old_path,
+bool NetworkFetcher::RenameToAppId(const GURL& url,
+                                   const base::FilePath& old_path,
                                    base::FilePath* new_path) {
   std::string app_id;
   if (!ComputeAppId(old_path, &app_id))
     return false;
 
+  // Using a hash of the url as a directory to prevent a race when the same
+  // bytes are downloaded from 2 different urls.
+  std::string dirname = base::HexEncode(
+      crypto::SHA256HashString(url.spec()).data(), crypto::kSHA256Length);

[Nick Bray (chromium), 2015/03/19 17:45:20]

It's worth commenting this dirname will not be consistent for a particular
application because spec contains junk that is not considered part of a
canonical application name.  It's the leaf file name that needs to be consistent
for gdb serve. Comment / crosslink to a URL why we need this consistent naming,
too.

It also makes me wonder if there's a race where we have two sources trying to
connect to the same app at the same time.

[qsr, 2015/03/20 11:50:48]

done

+
   base::FilePath temp_dir;
   base::GetTempDir(&temp_dir);
+  base::FilePath app_dir = temp_dir.Append(dirname);
+  bool result = base::CreateDirectoryAndGetError(app_dir, nullptr);

[Nick Bray (chromium), 2015/03/19 17:45:20]

Comment on leaking the directory and why.

[qsr, 2015/03/20 11:50:49]

Done.

+  DCHECK(result);
   std::string unique_name = base::StringPrintf("%s.mojo", app_id.c_str());
-  *new_path = temp_dir.Append(unique_name);
+  *new_path = app_dir.Append(unique_name);
   return base::Move(old_path, *new_path);
 }
 
 void NetworkFetcher::CopyCompleted(
     base::Callback<void(const base::FilePath&, bool)> callback,
     bool success) {
-  // The copy completed, now move to $TMP/$APP_ID.mojo before the dlopen.
   if (success) {
-    success = false;
-    base::FilePath new_path;
-    if (RenameToAppId(path_, &new_path)) {
-      if (base::PathExists(new_path)) {
-        path_ = new_path;
-        success = true;
-        RecordCacheToURLMapping(path_, url_);
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kPredictableAppFilenames)) {
+      // The copy completed, now move to $TMP/$APP_ID.mojo before the dlopen.
+      success = false;
+      base::FilePath new_path;
+      if (RenameToAppId(url_, path_, &new_path)) {
+        if (base::PathExists(new_path)) {
+          path_ = new_path;
+          success = true;
+        }
       }
     }
   }
 
+  if (success)
+    RecordCacheToURLMapping(path_, url_);
+
   base::MessageLoop::current()->PostTask(FROM_HERE,
                                          base::Bind(callback, path_, success));
 }
 
 void NetworkFetcher::AsPath(
     base::TaskRunner* task_runner,
     base::Callback<void(const base::FilePath&, bool)> callback) {
   if (!path_.empty() || !response_) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(callback, path_, base::PathExists(path_)));
@@ skipping 51 matching lines @@
     loader_callback_.Run(make_scoped_ptr<Fetcher>(NULL));
     return;
   }
 
   response_ = response.Pass();
   loader_callback_.Run(make_scoped_ptr(this));
 }
 
 }  // namespace shell
 }  // namespace mojo