Skip to content

Commit

Permalink
Fix a bug where the response cache could be corrupted.
Browse files Browse the repository at this point in the history
When streaming a response, we copy data from our buffer to the cached file
on disk. Unfortunately we were copying N bytes from the front of the buffer
when we wanted N bytes from the back of the buffer.

Typically these are the same, but certain access patterns can cause them
to be different, corrpting the cached file on disk.

This was uncovered by migrating the cache tests from operating on
HttpURLConnection's API to our new API.
  • Loading branch information
squarejesse committed Oct 27, 2014
1 parent c9cc836 commit f80b1f3
Show file tree
Hide file tree
Showing 11 changed files with 2,749 additions and 845 deletions.
1,537 changes: 774 additions & 763 deletions okhttp-tests/src/test/java/com/squareup/okhttp/CacheTest.java

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,17 @@
import com.squareup.okhttp.internal.tls.OkHostnameVerifier;
import java.io.IOException;
import java.net.Authenticator;
import java.net.CacheRequest;
import java.net.CacheResponse;
import java.net.CookieHandler;
import java.net.CookieManager;
import java.net.ProxySelector;
import java.net.ResponseCache;
import java.net.URI;
import java.net.URLConnection;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import javax.net.SocketFactory;
import org.junit.After;
import org.junit.Test;
Expand Down Expand Up @@ -95,9 +101,17 @@ public final class OkHttpClientTest {
assertNull(client.getCache());
}

@Test public void copyWithDefaultsDoesNotHonorGlobalResponseCache() throws Exception {
ResponseCache responseCache = new AbstractResponseCache();
ResponseCache.setDefault(responseCache);
@Test public void copyWithDefaultsDoesNotHonorGlobalResponseCache() {
ResponseCache.setDefault(new ResponseCache() {
@Override public CacheResponse get(URI uri, String requestMethod,
Map<String, List<String>> requestHeaders) throws IOException {
throw new AssertionError();
}

@Override public CacheRequest put(URI uri, URLConnection connection) {
throw new AssertionError();
}
});

OkHttpClient client = new OkHttpClient().copyWithDefaults();
assertNull(client.internalCache());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package com.squareup.okhttp.internal.http;

import com.squareup.okhttp.AbstractResponseCache;
import com.squareup.okhttp.Cache;
import com.squareup.okhttp.Challenge;
import com.squareup.okhttp.ConnectionConfiguration;
Expand All @@ -33,7 +32,6 @@
import com.squareup.okhttp.internal.SingleInetAddressNetwork;
import com.squareup.okhttp.internal.SslContextBuilder;
import com.squareup.okhttp.internal.Util;
import com.squareup.okhttp.internal.huc.CacheAdapter;
import com.squareup.okhttp.mockwebserver.MockResponse;
import com.squareup.okhttp.mockwebserver.MockWebServer;
import com.squareup.okhttp.mockwebserver.RecordedRequest;
Expand All @@ -43,7 +41,6 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.net.Authenticator;
import java.net.CacheRequest;
import java.net.ConnectException;
import java.net.HttpRetryException;
import java.net.HttpURLConnection;
Expand All @@ -70,7 +67,6 @@
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.zip.GZIPInputStream;
import javax.net.SocketFactory;
import javax.net.ssl.HttpsURLConnection;
Expand Down Expand Up @@ -2004,8 +2000,7 @@ private void testResponseRedirectedWithPost(int redirectCode, TransferKind trans
}

@Test public void redirectedPostStripsRequestBodyHeaders() throws Exception {
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
server.enqueue(new MockResponse().setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.addHeader("Location: /page2"));
server.enqueue(new MockResponse().setBody("Page 2"));
server.play();
Expand Down Expand Up @@ -2430,33 +2425,6 @@ private void testFlushAfterStreamTransmitted(TransferKind transferKind) throws I
}
}

/** Don't explode if the cache returns a null body. http://b/3373699 */
@Test public void responseCacheReturnsNullOutputStream() throws Exception {
final AtomicBoolean aborted = new AtomicBoolean();
Internal.instance.setCache(client.client(), new CacheAdapter(new AbstractResponseCache() {
@Override public CacheRequest put(URI uri, URLConnection connection) throws IOException {
return new CacheRequest() {
@Override public void abort() {
aborted.set(true);
}

@Override public OutputStream getBody() throws IOException {
return null;
}
};
}
}));

server.enqueue(new MockResponse().setBody("abcdef"));
server.play();

HttpURLConnection connection = client.open(server.getUrl("/"));
InputStream in = connection.getInputStream();
assertEquals("abc", readAscii(in, 3));
in.close();
assertFalse(aborted.get()); // The best behavior is ambiguous, but RI 6 doesn't abort here
}

/** http://code.google.com/p/android/issues/detail?id=14562 */
@Test public void readAfterLastByte() throws Exception {
server.enqueue(new MockResponse().setBody("ABC")
Expand Down
Loading

0 comments on commit f80b1f3

Please sign in to comment.