From 5ba69740a47857fcebd36866e07963ba798269d5 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 1 Mar 2013 11:18:38 -0800 Subject: Fix race conditions around filename claiming. When multiple downloads are running in parallel, they can end up claiming the same filename and clobber over each other. This change introduces locking around filename generation, and touches the claimed filename so other threads fail the File.exists() check and keep looking. Tests to verify. Bug: 8255596 Change-Id: Ie75ed047c199cf679832c75159056ca036eac18d --- src/com/android/providers/downloads/Helpers.java | 30 +++++++++----- .../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> 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 d : downloads) { + d.first.waitForStatus(DownloadManager.STATUS_SUCCESSFUL, startMillis); + } + + // Ensure that contents are clean and filenames unique + final Set seenFiles = Sets.newHashSet(); + + for (Pair 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); + } + } } -- cgit v1.2.3