Skip to content

Commit

Permalink
Fix possible NPE when trying to generate source ids for remote peer t… (
Browse files Browse the repository at this point in the history 
#631)

…o use.

Motivation:

We need to ensure we check if we already have some source id before
trying to use the source id to generate more ids for the remote peer to
use otherwhise we might see a NPE:

```
11:58:38.663 [nioEventLoopGroup-2-1] DEBUG io.netty.incubator.codec.quic.QuicheQuicCodec -- Error while processing QUIC packet
java.lang.NullPointerException: Cannot read field "connId" because "this.localIdAdrr" is null
	at io.netty.incubator.codec.quic.QuicheQuicChannel.newSourceConnectionIds(QuicheQuicChannel.java:917)
	at io.netty.incubator.codec.quic.QuicheQuicServerCodec.channelRecv(QuicheQuicServerCodec.java:110)
	at io.netty.incubator.codec.quic.QuicheQuicCodec$QuicCodecHeaderProcessor.process(QuicheQuicCodec.java:293)
	at io.netty.incubator.codec.quic.QuicHeaderParser.parse(QuicHeaderParser.java:127)
	at io.netty.incubator.codec.quic.QuicheQuicCodec.handleQuicPacket(QuicheQuicCodec.java:164)
	at io.netty.incubator.codec.quic.QuicheQuicCodec.channelRead(QuicheQuicCodec.java:155)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
	at io.netty.channel.nio.AbstractNioMessageChannel$NioMessageUnsafe.read(AbstractNioMessageChannel.java:97)
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:788)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:724)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:650)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:562)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:1583)
```

Modifications:

Always retrieve the source id and check for null without caching it.
This is also needed as the source id might change during the life-time
of the connection

Result:

No more NPE
  • Loading branch information
@normanmaurer
normanmaurer authored Dec 1, 2023
1 parent 4c561da commit c85d975
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 59 deletions.
21 changes: 9 additions & 12 deletions codec-classes-quic/src/main/java/io/netty/incubator/codec/quic/QuicheQuicChannel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,6 @@ public void operationComplete(ChannelFuture channelFuture) {
private volatile boolean timedOut;
private volatile String traceId;
private volatile QuicheQuicConnection connection;
private volatile QuicConnectionAddress remoteIdAddr;
private volatile QuicConnectionAddress localIdAdrr;

private static final AtomicLongFieldUpdater<QuicheQuicChannel> UNI_STREAMS_LEFT_UPDATER =
AtomicLongFieldUpdater.newUpdater(QuicheQuicChannel.class, "uniStreamsLeft");
Expand Down Expand Up @@ -534,12 +532,14 @@ protected boolean isCompatible(EventLoop eventLoop) {

@Override
protected SocketAddress localAddress0() {
return localIdAdrr;
QuicheQuicConnection connection = this.connection;
return connection == null ? null : connection.sourceId();
}

@Override
protected SocketAddress remoteAddress0() {
return remoteIdAddr;
QuicheQuicConnection connection = this.connection;
return connection == null ? null : connection.destinationId();
}

@Override
Expand Down Expand Up @@ -912,9 +912,13 @@ List<ByteBuffer> newSourceConnectionIds(
// is the reason why we might need to call connectionSendAndFlush().
int left = Quiche.quiche_conn_scids_left(connAddr);
if (left > 0) {
QuicConnectionAddress sourceAddr = connection.sourceId();
if (sourceAddr == null) {
return Collections.emptyList();
}
List<ByteBuffer> generatedIds = new ArrayList<>(left);
boolean sendAndFlush = false;
ByteBuffer key = localIdAdrr.connId.duplicate();
ByteBuffer key = sourceAddr.connId.duplicate();
ByteBuf connIdBuffer = alloc().directBuffer(key.remaining());

byte[] resetTokenArray = new byte[Quic.RESET_TOKEN_LEN];
Expand Down Expand Up @@ -1767,7 +1771,6 @@ private boolean handlePendingChannelActive() {
if (state == OPEN && Quiche.quiche_conn_is_established(connAddr)) {
// We didn't notify before about channelActive... Update state and fire the event.
state = ACTIVE;
initAddresses(connection);

pipeline().fireChannelActive();
notifyAboutHandshakeCompletionIfNeeded(null);
Expand All @@ -1777,7 +1780,6 @@ private boolean handlePendingChannelActive() {
ChannelPromise promise = connectPromise;
connectPromise = null;
state = ACTIVE;
initAddresses(connection);

boolean promiseSet = promise.trySuccess();
pipeline().fireChannelActive();
Expand All @@ -1792,11 +1794,6 @@ private boolean handlePendingChannelActive() {
return false;
}

private void initAddresses(QuicheQuicConnection connection) {
localIdAdrr = connection.sourceId();
remoteIdAddr = connection.destinationId();
}

private void fireDatagramExtensionEvent() {
long connAddr = connection.address();
int len = Quiche.quiche_conn_dgram_max_writable_len(connAddr);
Expand Down
47 changes: 0 additions & 47 deletions codec-native-quic/src/test/java/io/netty/incubator/codec/quic/QuicChannelConnectTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -515,53 +515,6 @@ public void testConnectWithoutTokenValidation(Executor executor) throws Throwabl
}
}

@ParameterizedTest
@MethodSource("newSslTaskExecutors")
@Timeout(value = 5000, unit = TimeUnit.MILLISECONDS)
public void testConnectAndGetAddressesAfterClose(Executor executor) throws Throwable {
AtomicReference<QuicChannel> acceptedRef = new AtomicReference<>();
Channel server = QuicTestUtils.newServer(executor,
new ChannelInboundHandlerAdapter() {
@Override
public void channelActive(ChannelHandlerContext ctx) throws Exception {
acceptedRef.set((QuicChannel) ctx.channel());
super.channelActive(ctx);
}
},
new ChannelInboundHandlerAdapter());
InetSocketAddress address = (InetSocketAddress) server.localAddress();
Channel channel = QuicTestUtils.newClient(executor);
try {
QuicChannel quicChannel = QuicTestUtils.newQuicChannelBootstrap(channel)
.handler(new ChannelInboundHandlerAdapter())
.streamHandler(new ChannelInboundHandlerAdapter())
.remoteAddress(address)
.connect()
.get();
quicChannel.close().sync();
ChannelFuture closeFuture = quicChannel.closeFuture().await();
assertTrue(closeFuture.isSuccess());

// Check if we also can access these after the channel was closed.
assertNotNull(quicChannel.localAddress());
assertNotNull(quicChannel.remoteAddress());

QuicChannel accepted;
while ((accepted = acceptedRef.get()) == null) {
Thread.sleep(50);
}
// Check if we also can access these after the channel was closed.
assertNotNull(accepted.localAddress());
assertNotNull(accepted.remoteAddress());
} finally {
server.close().sync();
// Close the parent Datagram channel as well.
channel.close().sync();

shutdown(executor);
}
}

@ParameterizedTest
@MethodSource("newSslTaskExecutors")
public void testConnectWith0RTT(Executor executor) throws Throwable {
Expand Down

0 comments on commit c85d975

Please sign in to comment.