From e6089c9630daf60ea57b930d1d66d9b92980c0d6 Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Tue, 8 Sep 2020 06:25:33 -0700 Subject: [PATCH] Fix races in the HTTP upload and download handlers The user promise has a callback that returns the connection to the pool. If the server returns a 'connection: close' HTTP header, then this can currently happen before the connection is closed, in which case the client attempts to reuse the connection, which - of course - fails. This changes the ordering to close the connection *before* completing the user promise. This is at least a partial fix for the linked issue. It is unclear if this is the root cause for all the reported failure modes. Progress on #10159. Change-Id: I2897e55c6edda592a6fb5755ddcccd1a89cde528 Closes #12055. Change-Id: I2897e55c6edda592a6fb5755ddcccd1a89cde528 PiperOrigin-RevId: 330496714 --- .../lib/remote/http/AbstractHttpHandler.java | 6 ---- .../lib/remote/http/HttpDownloadHandler.java | 22 +++++++++++---- .../lib/remote/http/HttpUploadHandler.java | 28 +++++++++++-------- .../remote/http/HttpUploadHandlerTest.java | 2 +- 4 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/AbstractHttpHandler.java b/src/main/java/com/google/devtools/build/lib/remote/http/AbstractHttpHandler.java index e82e236433cb24..60403a6c780f5d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/http/AbstractHttpHandler.java +++ b/src/main/java/com/google/devtools/build/lib/remote/http/AbstractHttpHandler.java @@ -59,12 +59,6 @@ protected void failAndResetUserPromise(Throwable t) { userPromise = null; } - @SuppressWarnings("FutureReturnValueIgnored") - protected void succeedAndResetUserPromise() { - userPromise.setSuccess(); - userPromise = null; - } - protected void addCredentialHeaders(HttpRequest request, URI uri) throws IOException { String userInfo = uri.getUserInfo(); if (userInfo != null) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/HttpDownloadHandler.java b/src/main/java/com/google/devtools/build/lib/remote/http/HttpDownloadHandler.java index 37012aa62af511..50d83d138a1d66 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/http/HttpDownloadHandler.java +++ b/src/main/java/com/google/devtools/build/lib/remote/http/HttpDownloadHandler.java @@ -170,27 +170,37 @@ private HttpRequest buildRequest(String path, String host) { } private void succeedAndReset(ChannelHandlerContext ctx) { + // All resets must happen *before* completing the user promise. Otherwise there is a race + // condition, where this handler can be reused even though it is closed. In addition, if reset + // calls ctx.close(), then that triggers a call to AbstractHttpHandler.channelInactive, which + // attempts to close the user promise. + ChannelPromise promise = userPromise; + userPromise = null; try { - succeedAndResetUserPromise(); - } finally { reset(ctx); + } finally { + promise.setSuccess(); } } @SuppressWarnings("FutureReturnValueIgnored") private void failAndClose(Throwable t, ChannelHandlerContext ctx) { + ChannelPromise promise = userPromise; + userPromise = null; try { - failAndResetUserPromise(t); - } finally { ctx.close(); + } finally { + promise.setFailure(t); } } private void failAndReset(Throwable t, ChannelHandlerContext ctx) { + ChannelPromise promise = userPromise; + userPromise = null; try { - failAndResetUserPromise(t); - } finally { reset(ctx); + } finally { + promise.setFailure(t); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/HttpUploadHandler.java b/src/main/java/com/google/devtools/build/lib/remote/http/HttpUploadHandler.java index 22ea2597bf823f..21e1a1f8b19506 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/http/HttpUploadHandler.java +++ b/src/main/java/com/google/devtools/build/lib/remote/http/HttpUploadHandler.java @@ -56,8 +56,17 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpResponse response failAndClose(new IOException("Failed to parse the HTTP response."), ctx); return; } + + checkState(userPromise != null, "response before request"); + ChannelPromise promise = userPromise; + userPromise = null; + // Connection reset must happen *before* completing the user promise. Otherwise there is a race + // condition, where this handler can be reused even though it is closed. try { - checkState(userPromise != null, "response before request"); + if (!HttpUtil.isKeepAlive(response)) { + ctx.close(); + } + } finally { if (!response.status().equals(HttpResponseStatus.OK) && !response.status().equals(HttpResponseStatus.ACCEPTED) && !response.status().equals(HttpResponseStatus.CREATED) @@ -69,13 +78,9 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpResponse response response.content().readBytes(data); errorMsg += "\n" + new String(data, HttpUtil.getCharset(response)); } - failAndResetUserPromise(new HttpException(response, errorMsg, null)); + promise.setFailure(new HttpException(response, errorMsg, null)); } else { - succeedAndResetUserPromise(); - } - } finally { - if (!HttpUtil.isKeepAlive(response)) { - ctx.close(); + promise.setSuccess(); } } } @@ -144,10 +149,9 @@ private HttpChunkedInput buildBody(UploadCommand msg) { @SuppressWarnings("FutureReturnValueIgnored") private void failAndClose(Throwable t, ChannelHandlerContext ctx) { - try { - failAndResetUserPromise(t); - } finally { - ctx.close(); - } + // All resets must happen *before* completing the user promise. Otherwise there is a race + // condition, where this handler can be reused even though it is closed. + ctx.close(); + failAndResetUserPromise(t); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/http/HttpUploadHandlerTest.java b/src/test/java/com/google/devtools/build/lib/remote/http/HttpUploadHandlerTest.java index b4cdd17d383c07..9bb8d379fba486 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/http/HttpUploadHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/http/HttpUploadHandlerTest.java @@ -40,7 +40,7 @@ /** Tests for {@link HttpUploadHandler}. */ @RunWith(JUnit4.class) -public class HttpUploadHandlerTest extends AbstractHttpHandlerTest { +public class HttpUploadHandlerTest { private static final URI CACHE_URI = URI.create("http://storage.googleapis.com:80/cache-bucket");