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

Move EmptyHttpHeaders.INSTANCE initialization to inner class.

Browse Source 
Motivation:

If the HttpUtil class is initialized before HttpHeaders or
EmptyHttpHeaders, EmptyHttpHeaders.INSTANCE will be null. This
can lead to NPEs in code that relies on this field being
non-null. One example is the
LastHttpContent.EMPTY_LAST_CONTENT.trailingHeaders method.

Modifications:

- Move HttpUtil.EMPTY_HEADERS to a private static final inner class
  of EmptyHttpHeaders called InstanceInitializer.
- Add tests, that when run in isolation, validate the fix for the issue.

Result:

Any initialization order of HttpUtil, EmptyHttpHeaders or
HttpHeaders will result in EmptyHttpHeaders.INSTANCE being initialized
correctly.
This commit is contained in:
Dan McNulty 2017-11-15 08:54:59 -06:00 committed by Norman Maurer
parent d976dc108d
commit 48b4502d1d
4 changed files with 155 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
codec-http/src/main/java/io/netty/handler/codec/http/EmptyHttpHeaders.java
View File

@ -29,15 +29,15 @@ public class EmptyHttpHeaders extends HttpHeaders {
public static final EmptyHttpHeaders INSTANCE = instance();
/**
* @see InstanceInitializer#EMPTY_HEADERS
* @deprecated Use {@link EmptyHttpHeaders#INSTANCE}
* <p>
* This is needed to break a cyclic static initialization loop between {@link HttpHeaders} and
* {@link EmptyHttpHeaders}.
* @see HttpUtil#EMPTY_HEADERS
* This is needed to break a cyclic static initialization loop between {@link HttpHeaders} and {@link
* EmptyHttpHeaders}.
*/
@Deprecated
static EmptyHttpHeaders instance() {
return HttpUtil.EMPTY_HEADERS;
return InstanceInitializer.EMPTY_HEADERS;
}
protected EmptyHttpHeaders() {
@ -167,4 +167,22 @@ public class EmptyHttpHeaders extends HttpHeaders {
public Iterator<Entry<CharSequence, CharSequence>> iteratorCharSequence() {
return EMPTY_CHARS_ITERATOR;
}
/**
* This class is needed to break a cyclic static initialization loop between {@link HttpHeaders} and
* {@link EmptyHttpHeaders}.
*/
@Deprecated
private static final class InstanceInitializer {
/**
* The instance is instantiated here to break the cyclic static initialization between {@link EmptyHttpHeaders}
* and {@link HttpHeaders}. The issue is that if someone accesses {@link EmptyHttpHeaders#INSTANCE} before
* {@link HttpHeaders#EMPTY_HEADERS} then {@link HttpHeaders#EMPTY_HEADERS} will be {@code null}.
*/
@Deprecated
private static final EmptyHttpHeaders EMPTY_HEADERS = new EmptyHttpHeaders();
private InstanceInitializer() {
}
}
}

10
codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java
View File

@ -29,15 +29,7 @@ import java.util.List;
* Utility methods useful in the HTTP context.
*/
public final class HttpUtil {
/**
* @deprecated Use {@link EmptyHttpHeaders#INSTANCE}
* <p>
* The instance is instantiated here to break the cyclic static initialization between {@link EmptyHttpHeaders} and
* {@link HttpHeaders}. The issue is that if someone accesses {@link EmptyHttpHeaders#INSTANCE} before
* {@link HttpHeaders#EMPTY_HEADERS} then {@link HttpHeaders#EMPTY_HEADERS} will be {@code null}.
*/
@Deprecated
static final EmptyHttpHeaders EMPTY_HEADERS = new EmptyHttpHeaders();
private static final AsciiString CHARSET_EQUALS = AsciiString.of(HttpHeaderValues.CHARSET + "=");
private static final AsciiString SEMICOLON = AsciiString.cached(";");

43
codec-http/src/test/java/io/netty/handler/codec/http/EmptyHttpHeadersInitializationTest.java Normal file
View File

@ -0,0 +1,43 @@
/*
* Copyright 2017 The Netty Project
*
* The Netty Project licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package io.netty.handler.codec.http;
import org.junit.Test;
import static org.junit.Assert.*;
/**
* A test to validate that either order of initialization of the {@link EmptyHttpHeaders#INSTANCE} and
* {@link HttpHeaders#EMPTY_HEADERS} field results in both fields being non-null.
*
* Since this is testing static initialization, the tests might not actually test anything, except
* when run in isolation.
*/
public class EmptyHttpHeadersInitializationTest {
@Test
public void testEmptyHttpHeadersFirst() {
assertNotNull(EmptyHttpHeaders.INSTANCE);
assertNotNull(HttpHeaders.EMPTY_HEADERS);
}
@Test
public void testHttpHeadersFirst() {
assertNotNull(HttpHeaders.EMPTY_HEADERS);
assertNotNull(EmptyHttpHeaders.INSTANCE);
}
}

89
codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestEncoderTest.java
View File

@ -18,6 +18,8 @@ package io.netty.handler.codec.http;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.channel.embedded.EmbeddedChannel;
import io.netty.handler.codec.DecoderResult;
import io.netty.util.CharsetUtil;
import io.netty.util.IllegalReferenceCountException;
import org.junit.Test;
@ -189,4 +191,91 @@ public class HttpRequestEncoderTest {
lastContent.release();
assertFalse(channel.finish());
}
/**
* A test that checks for a NPE that would occur if when processing {@link LastHttpContent#EMPTY_LAST_CONTENT}
* when a certain initialization order of {@link EmptyHttpHeaders} would occur.
*/
@Test
public void testForChunkedRequestNpe() throws Exception {
EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestEncoder());
assertTrue(channel.writeOutbound(new CustomHttpRequest()));
assertTrue(channel.writeOutbound(new DefaultHttpContent(Unpooled.copiedBuffer("test", CharsetUtil.US_ASCII))));
assertTrue(channel.writeOutbound(LastHttpContent.EMPTY_LAST_CONTENT));
assertTrue(channel.finishAndReleaseAll());
}
/**
* This class is required to triggered the desired initialization order of {@link EmptyHttpHeaders}.
* If {@link DefaultHttpRequest} is used, the {@link HttpHeaders} class will be initialized before {@link HttpUtil}
* and the test won't trigger the original issue.
*/
private static final class CustomHttpRequest implements HttpRequest {
@Override
public DecoderResult decoderResult() {
return DecoderResult.SUCCESS;
}
@Override
public void setDecoderResult(DecoderResult result) {
}
@Override
public DecoderResult getDecoderResult() {
return decoderResult();
}
@Override
public HttpVersion getProtocolVersion() {
return HttpVersion.HTTP_1_1;
}
@Override
public HttpVersion protocolVersion() {
return getProtocolVersion();
}
@Override
public HttpHeaders headers() {
DefaultHttpHeaders headers = new DefaultHttpHeaders();
headers.add("Transfer-Encoding", "chunked");
return headers;
}
@Override
public HttpMethod getMethod() {
return HttpMethod.POST;
}
@Override
public HttpMethod method() {
return getMethod();
}
@Override
public HttpRequest setMethod(HttpMethod method) {
return this;
}
@Override
public String getUri() {
return "/";
}
@Override
public String uri() {
return "/";
}
@Override
public HttpRequest setUri(String uri) {
return this;
}
@Override
public HttpRequest setProtocolVersion(HttpVersion version) {
return this;
}
}
}