summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
-rw-r--r--src/com/android/providers/downloads/Helpers.java30
-rw-r--r--tests/src/com/android/providers/downloads/ThreadingTest.java46
2 files changed, 66 insertions, 10 deletions
diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java
index 593e28b..3320555 100644
--- a/src/com/android/providers/downloads/Helpers.java
+++ b/src/com/android/providers/downloads/Helpers.java
@@ -17,10 +17,6 @@
package com.android.providers.downloads;
import android.content.Context;
-import android.content.Intent;
-import android.content.pm.PackageManager;
-import android.content.pm.ResolveInfo;
-import android.net.NetworkInfo;
import android.net.Uri;
import android.os.Environment;
import android.os.SystemClock;
@@ -29,6 +25,7 @@ import android.util.Log;
import android.webkit.MimeTypeMap;
import java.io.File;
+import java.io.IOException;
import java.util.Random;
import java.util.Set;
import java.util.regex.Matcher;
@@ -44,6 +41,8 @@ public class Helpers {
private static final Pattern CONTENT_DISPOSITION_PATTERN =
Pattern.compile("attachment;\\s*filename\\s*=\\s*\"([^\"]*)\"");
+ private static final Object sUniqueLock = new Object();
+
private Helpers() {
}
@@ -92,10 +91,10 @@ public class Helpers {
destination);
}
storageManager.verifySpace(destination, path, contentLength);
- path = getFullPath(path, mimeType, destination, base);
if (DownloadDrmHelper.isDrmConvertNeeded(mimeType)) {
path = DownloadDrmHelper.modifyDrmFwLockFileExtension(path);
}
+ path = getFullPath(path, mimeType, destination, base);
return path;
}
@@ -132,7 +131,21 @@ public class Helpers {
if (Constants.LOGVV) {
Log.v(Constants.TAG, "target file: " + filename + extension);
}
- return chooseUniqueFilename(destination, filename, extension, recoveryDir);
+
+ synchronized (sUniqueLock) {
+ final String path = chooseUniqueFilenameLocked(
+ destination, filename, extension, recoveryDir);
+
+ // Claim this filename inside lock to prevent other threads from
+ // clobbering us. We're not paranoid enough to use O_EXCL.
+ try {
+ new File(path).createNewFile();
+ } catch (IOException e) {
+ throw new StopRequestException(Downloads.Impl.STATUS_FILE_ERROR,
+ "Failed to create target file " + path, e);
+ }
+ return path;
+ }
}
private static String chooseFilename(String url, String hint, String contentDisposition,
@@ -282,11 +295,8 @@ public class Helpers {
return extension;
}
- private static String chooseUniqueFilename(int destination, String filename,
+ private static String chooseUniqueFilenameLocked(int destination, String filename,
String extension, boolean recoveryDir) throws StopRequestException {
- // TODO: switch to actually creating the file here, otherwise we expose
- // ourselves to race conditions.
-
String fullFilename = filename + extension;
if (!new File(fullFilename).exists()
&& (!recoveryDir ||
diff --git a/tests/src/com/android/providers/downloads/ThreadingTest.java b/tests/src/com/android/providers/downloads/ThreadingTest.java
index fcb3329..920f703 100644
--- a/tests/src/com/android/providers/downloads/ThreadingTest.java
+++ b/tests/src/com/android/providers/downloads/ThreadingTest.java
@@ -20,6 +20,15 @@ import static java.net.HttpURLConnection.HTTP_OK;
import android.app.DownloadManager;
import android.test.suitebuilder.annotation.LargeTest;
+import android.util.Pair;
+
+import com.google.android.collect.Lists;
+import com.google.android.collect.Sets;
+import com.google.mockwebserver.MockResponse;
+import com.google.mockwebserver.SocketPolicy;
+
+import java.util.List;
+import java.util.Set;
/**
* Download manager tests that require multithreading.
@@ -48,4 +57,41 @@ public class ThreadingTest extends AbstractPublicApiTest {
Thread.sleep(10);
}
}
+
+ public void testFilenameRace() throws Exception {
+ final List<Pair<Download, String>> downloads = Lists.newArrayList();
+
+ // Request dozen files at once with same name
+ for (int i = 0; i < 12; i++) {
+ final String body = "DOWNLOAD " + i + " CONTENTS";
+ enqueueResponse(new MockResponse().setResponseCode(HTTP_OK).setBody(body)
+ .setHeader("Content-type", "text/plain")
+ .setSocketPolicy(SocketPolicy.DISCONNECT_AT_END));
+
+ final Download d = enqueueRequest(getRequest());
+ downloads.add(Pair.create(d, body));
+ }
+
+ // Kick off downloads in parallel
+ final long startMillis = mSystemFacade.currentTimeMillis();
+ startService(null);
+
+ for (Pair<Download,String> d : downloads) {
+ d.first.waitForStatus(DownloadManager.STATUS_SUCCESSFUL, startMillis);
+ }
+
+ // Ensure that contents are clean and filenames unique
+ final Set<String> seenFiles = Sets.newHashSet();
+
+ for (Pair<Download, String> d : downloads) {
+ final String file = d.first.getStringField(DownloadManager.COLUMN_LOCAL_FILENAME);
+ if (!seenFiles.add(file)) {
+ fail("Another download already claimed " + file);
+ }
+
+ final String expected = d.second;
+ final String actual = d.first.getContents();
+ assertEquals(expected, actual);
+ }
+ }
}