Skip to content

[ROCKETMQ-34] Potential NPE in NettyConnetManageHandler#connect#30

Closed
shroman wants to merge 2 commits into
apache:masterfrom
shroman:ROCKETMQ-34
Closed

[ROCKETMQ-34] Potential NPE in NettyConnetManageHandler#connect#30
shroman wants to merge 2 commits into
apache:masterfrom
shroman:ROCKETMQ-34

Conversation

@shroman

@shroman shroman commented Jan 6, 2017

Copy link
Copy Markdown
Contributor

@shroman

shroman commented Jan 6, 2017

Copy link
Copy Markdown
Contributor Author

@vongosling @zhouxinyu Easy fix. Please review.

@zhouxinyu

Copy link
Copy Markdown
Member

Easy fix indeed, thanks @shroman , please @vongosling @stevenschew review.

@stevenschew

Copy link
Copy Markdown
Contributor

review Ok

@vintagewang

Copy link
Copy Markdown
Contributor
        public void connect(ChannelHandlerContext ctx, SocketAddress remoteAddress, SocketAddress localAddress, ChannelPromise promise)
            throws Exception {
            final String local = localAddress == null ? "UNKNOW" : localAddress.toString();
            final String remote = remoteAddress == null ? "UNKNOW" : remoteAddress.toString();
            log.info("NETTY CLIENT PIPELINE: CONNECT  {} => {}", local, remote);
            super.connect(ctx, remoteAddress, localAddress, promise);

            if (NettyRemotingClient.this.channelEventListener != null) {
                NettyRemotingClient.this.putNettyEvent(new NettyEvent(NettyEventType.CONNECT, remoteAddress.toString(), ctx.channel()));
            }
        }

Hi, @shroman is this super.connect dangerous ?

@vintagewang

Copy link
Copy Markdown
Contributor

image

review ok.

@shroman

shroman commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

@vintagewang super.connect can throw NPE when remoteAddress is null, but as far as I see in netty code, it is ok (notification is raised). And the client handles this in invoke* methods by throw new RemotingConnectException(addr); (for instance, NettyRemotingClient lines 518-519).
To summarize, it looks safe.

@asfgit asfgit closed this in 776911d Jan 9, 2017
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020
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.

4 participants