Fix a possible IllegalReferenceCountException in WebSocket handshake

[ikhoon]

Motivation:

The following IllegalReferenceCountException could be raised during a HTTP/1 Websocket handshake.

01:54:00.031 [armeria-common-worker-kqueue-2-2] WARN  c.l.a.i.client.PendingExceptionUtil - [id: 0xd2fa853c, L:/10.31.29.254:65140 - R:vks-api-prod.linecorp-dev.com/10.120.97.166:443] Unexpected suppressed exception:
com.linecorp.armeria.common.ClosedSessionException: null
Caused by: io.netty.util.IllegalReferenceCountException: refCnt: 0, decrement: 1
	at io.netty.util.internal.ReferenceCountUpdater.toLiveRealRefCnt(ReferenceCountUpdater.java:83)
	at io.netty.util.internal.ReferenceCountUpdater.release(ReferenceCountUpdater.java:148)
	at io.netty.buffer.AbstractReferenceCountedByteBuf.release(AbstractReferenceCountedByteBuf.java:101)
	at io.netty.util.ReferenceCountUtil.release(ReferenceCountUtil.java:90)
	at com.linecorp.armeria.client.WebSocketHttp1ClientChannelHandler.channelRead(WebSocketHttp1ClientChannelHandler.java:246)
	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.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:436)
	at io.netty.handler.codec.ByteToMessageDecoder.handlerRemoved(ByteToMessageDecoder.java:266)
	at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.remove0(CombinedChannelDuplexHandler.java:608)
	at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.remove(CombinedChannelDuplexHandler.java:593)
	at io.netty.channel.CombinedChannelDuplexHandler.handlerRemoved(CombinedChannelDuplexHandler.java:181)
	at io.netty.channel.AbstractChannelHandlerContext.callHandlerRemoved(AbstractChannelHandlerContext.java:1138)
	at io.netty.channel.DefaultChannelPipeline.callHandlerRemoved0(DefaultChannelPipeline.java:637)
	at io.netty.channel.DefaultChannelPipeline.remove(DefaultChannelPipeline.java:477)
	at io.netty.channel.DefaultChannelPipeline.remove(DefaultChannelPipeline.java:429)
	at com.linecorp.armeria.client.WebSocketHttp1ClientChannelHandler.channelRead(WebSocketHttp1ClientChannelHandler.java:209)

• When 101 Switching Protocols response is received, WebSocketHttp1ClientChannelHandler immediately removes HttpClientCodec from the channel pipeline.

  armeria/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ClientChannelHandler.java

  Line 195 in 75d5af1

  pipeline.remove(HttpClientCodec.class);

• The removal may emit remaining buffers via channelRead(). https://github.com/netty/netty/blob/06a7e12df9118e3ffbeae3cdc50e638a999d93ec/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java#L266-L267
• WebSocket packets after the HTTP/1 WebSocket handshake are passed to WebSocketHttp1ClientChannelHandler, but the state is still NEEDS_HANDSHAKE_RESPONSE. As a result, the state machine becomes broken and the ByteBuf could be double released.

  armeria/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ClientChannelHandler.java

  Lines 165 to 167 in 75d5af1

  	case NEEDS_HANDSHAKE_RESPONSE:
  	if (!(msg instanceof HttpObject)) {
  	ctx.fireChannelRead(msg);

Modifications:

• Retain a message before passing the next ChannelHandler because it is always released.
• Remove HttpClientCodec after EMPTY_LAST_CONTENT is received and state becomes UPGRADE_COMPLTE to handle pending messages in the buffer correctly.

Result:

IllegalReferenceCountException no longer occurs during WebSocket handshake.

[github-actions]

🔍 Build Scan® (commit: 045212d)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/ve5znbm67qemu
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/yibpj6rdjtwdo
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/5vgwtawbht6aq
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/vt43hnlrxhqbi
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/dute3opzronws
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/nvepsoyzy65fy
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/67k36uorppobg
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/pfbte4cfh53vu

[minwoox]

Nice catch! Thanks!

[jrhee17]

Very nice! Thanks for the analysis + fix! 🙇 👍 🙇

[ikhoon]

Thanks for all the reviews. 🙇‍♂️🙇‍♂️