Skip to content

Conversation

@JoeCqupt
Copy link
Contributor

@JoeCqupt JoeCqupt commented Aug 14, 2024

reproduce code

    @Test
    public void testTcpDecodeIllegalPacket1() {
        Codec codec = mock(Codec.class);
        doThrow(TRpcException.newFrameException(ErrorCode.TRPC_CLIENT_DECODE_ERR, "the request protocol is not trpc"))
                .when(codec).decode(any(), any());


        ProtocolConfig protocolConfig = new ProtocolConfig();
        // set batchDecoder true
        protocolConfig.setBatchDecoder(true);
        NettyCodecAdapter nettyCodecAdapter = NettyCodecAdapter.createTcpCodecAdapter(codec, protocolConfig);

        ChannelHandler decoder = nettyCodecAdapter.getDecoder();
        EmbeddedChannel embeddedChannel = new EmbeddedChannel();
        embeddedChannel.pipeline().addLast(decoder);

        ByteBuf byteBuf = AbstractByteBufAllocator.DEFAULT.heapBuffer();
        byteBuf.writeBytes("testTcpDecodeIllegalPacket1".getBytes(StandardCharsets.UTF_8));

        // write illegal packet
        EmbeddedChannel tmpEmbeddedChannel = embeddedChannel;
        DecoderException decoderException = Assert.assertThrows(DecoderException.class, () -> {
            tmpEmbeddedChannel.writeInbound(byteBuf);
        });

        Assert.assertTrue(decoderException.getCause() instanceof TRpcException);

        TRpcException tRpcException = (TRpcException) decoderException.getCause();
        Assert.assertEquals(tRpcException.getCode(), ErrorCode.TRPC_CLIENT_DECODE_ERR);

        Assert.assertEquals(byteBuf.refCnt(), 0);
    }

when TcpDecoder#decode() throw exception . ByteBuf param not released
expect refcnt 0 , actual 1

Solution
when decode excpetion . skip all readable bytes let ByteToMessageDecoder release ByteBuf

@github-actions
Copy link

github-actions bot commented Aug 14, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@JoeCqupt
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

I have read the CLA Document and I hereby sign the CLA

@JoeCqupt
Copy link
Contributor Author

recheck

@codecov
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.34263%. Comparing base (a41c9aa) to head (9348276).
Report is 13 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@                  Coverage Diff                  @@
##                master         #54         +/-   ##
=====================================================
+ Coverage     69.27388%   69.34263%   +0.06874%     
- Complexity        4108        4111          +3     
=====================================================
  Files              428         428                 
  Lines            16774       16779          +5     
  Branches          1698        1700          +2     
=====================================================
+ Hits             11620       11635         +15     
+ Misses            4048        4039          -9     
+ Partials          1106        1105          -1     
Files Coverage Δ
...com/tencent/trpc/transport/netty/NettyChannel.java 59.37500% <100.00000%> (+2.70832%) ⬆️
...encent/trpc/transport/netty/NettyCodecAdapter.java 78.16092% <100.00000%> (+3.16092%) ⬆️

... and 7 files with indirect coverage changes

@JoeCqupt
Copy link
Contributor Author

@wardseptember PTAL

@JoeCqupt
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@wardseptember
Copy link
Collaborator

感谢贡献,我们评估一下

@wardseptember
Copy link
Collaborator

I have read the CLA Document and I hereby sign the CLA

你需要用github账户提交push分支,然后才能授权成功

liuzengh added a commit to trpc-group/cla-database that referenced this pull request Aug 20, 2024
@wardseptember wardseptember merged commit acdbb07 into trpc-group:master Aug 26, 2024
@wardseptember
Copy link
Collaborator

感谢贡献,已合入

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants