diff options
-rw-r--r-- | src/com/android/providers/downloads/Helpers.java | 30 | ||||
-rw-r--r-- | tests/src/com/android/providers/downloads/ThreadingTest.java | 46 |
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 593e28b8..33205557 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 fcb33290..920f703b 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); + } + } } |