andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix possible NPE when using HttpClientCodec (#9465)

Browse Source 
Motivation:

It was possible to produce a NPE when we for examples received more responses as requests as we did not check if the queue did not contain a method before trying to compare method names.

Modifications:

- Add extra null check
- Add unit tet

Result:

Fixes https://github.com/netty/netty/issues/9459
This commit is contained in:
Norman Maurer 2019-08-16 08:17:18 +02:00
parent e4d9a17ad9
commit 96f92929ab
2 changed files with 64 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

77
codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
View File

@ -160,7 +160,7 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
return; return;
} }
if (msg instanceof HttpRequest && !done) { if (msg instanceof HttpRequest) {
queue.offer(((HttpRequest) msg).method()); queue.offer(((HttpRequest) msg).method());
} }
@ -233,46 +233,49 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
// current response. // current response.
HttpMethod method = queue.poll(); HttpMethod method = queue.poll();
char firstChar = method.name().charAt(0); // If the remote peer did for example send multiple responses for one request (which is not allowed per
switch (firstChar) { // spec but may still be possible) method will be null so guard against it.
case 'H': if (method != null) {
// According to 4.3, RFC2616: char firstChar = method.name().charAt(0);
// All responses to the HEAD request method MUST NOT include a switch (firstChar) {
// message-body, even though the presence of entity-header fields case 'H':
// might lead one to believe they do. // According to 4.3, RFC2616:
if (HttpMethod.HEAD.equals(method)) { // All responses to the HEAD request method MUST NOT include a
return true; // message-body, even though the presence of entity-header fields
// might lead one to believe they do.
if (HttpMethod.HEAD.equals(method)) {
return true;
// The following code was inserted to work around the servers // The following code was inserted to work around the servers
// that behave incorrectly. It has been commented out // that behave incorrectly. It has been commented out
// because it does not work with well behaving servers. // because it does not work with well behaving servers.
// Please note, even if the 'Transfer-Encoding: chunked' // Please note, even if the 'Transfer-Encoding: chunked'
// header exists in the HEAD response, the response should // header exists in the HEAD response, the response should
// have absolutely no content. // have absolutely no content.
// //
//// Interesting edge case: //// Interesting edge case:
//// Some poorly implemented servers will send a zero-byte //// Some poorly implemented servers will send a zero-byte
//// chunk if Transfer-Encoding of the response is 'chunked'. //// chunk if Transfer-Encoding of the response is 'chunked'.
//// ////
//// return !msg.isChunked(); //// return !msg.isChunked();
}
break;
case 'C':
// Successful CONNECT request results in a response with empty body.
if (statusCode == 200) {
if (HttpMethod.CONNECT.equals(method)) {
// Proxy connection established - Parse HTTP only if configured by parseHttpAfterConnectRequest,
// else pass through.
if (!parseHttpAfterConnectRequest) {
done = true;
queue.clear();
} }
return true; break;
} case 'C':
// Successful CONNECT request results in a response with empty body.
if (statusCode == 200) {
if (HttpMethod.CONNECT.equals(method)) {
// Proxy connection established - Parse HTTP only if configured by
// parseHttpAfterConnectRequest, else pass through.
if (!parseHttpAfterConnectRequest) {
done = true;
queue.clear();
}
return true;
}
}
break;
} }
break;
} }
return super.isContentAlwaysEmpty(msg); return super.isContentAlwaysEmpty(msg);
} }

24
codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java
View File

@ -326,4 +326,28 @@ public class HttpClientCodecTest {
assertThat(ch.readInbound(), is(nullValue())); assertThat(ch.readInbound(), is(nullValue()));
} }
@Test
public void testMultipleResponses() {
String response = "HTTP/1.1 200 OK\r\n" +
"Content-Length: 0\r\n\r\n";
HttpClientCodec codec = new HttpClientCodec(4096, 8192, 8192, true);
EmbeddedChannel ch = new EmbeddedChannel(codec, new HttpObjectAggregator(1024));
HttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "http://localhost/");
assertTrue(ch.writeOutbound(request));
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(response, CharsetUtil.UTF_8)));
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(response, CharsetUtil.UTF_8)));
FullHttpResponse resp = ch.readInbound();
assertTrue(resp.decoderResult().isSuccess());
resp.release();
resp = ch.readInbound();
assertTrue(resp.decoderResult().isSuccess());
resp.release();
assertTrue(ch.finishAndReleaseAll());
}
} }