You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by jbertram <gi...@git.apache.org> on 2018/09/26 19:28:52 UTC
[GitHub] activemq-artemis pull request #2334: ARTEMIS-2098 potential NPE when decodin...
GitHub user jbertram opened a pull request:
https://github.com/apache/activemq-artemis/pull/2334
ARTEMIS-2098 potential NPE when decoding protocol
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-2098
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2334.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2334
----
commit 9283e6227581078c18d5b73261e8685d437e1cb0
Author: Justin Bertram <jb...@...>
Date: 2018-09-26T19:27:14Z
ARTEMIS-2098 potential NPE when decoding protocol
----
---
[GitHub] activemq-artemis pull request #2334: ARTEMIS-2098 potential NPE when decodin...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2334
---
[GitHub] activemq-artemis pull request #2334: ARTEMIS-2098 potential NPE when decodin...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2334#discussion_r220700987
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java ---
@@ -196,6 +196,10 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
}
ProtocolManager protocolManagerToUse = protocolMap.get(protocolToUse);
+ if (protocolManagerToUse == null) {
+ ActiveMQServerLogger.LOGGER.failedToFindProtocolManager(ctx.channel().remoteAddress().toString(), ctx.channel().localAddress().toString(), protocolToUse, protocolMap.keySet().toString());
--- End diff --
I performed manual testing to confirm the logging works as expected, but I'm not 100% sure these won't *ever* be null so checks are worth doing.
---
[GitHub] activemq-artemis pull request #2334: ARTEMIS-2098 potential NPE when decodin...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2334#discussion_r220697913
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java ---
@@ -196,6 +196,10 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
}
ProtocolManager protocolManagerToUse = protocolMap.get(protocolToUse);
+ if (protocolManagerToUse == null) {
+ ActiveMQServerLogger.LOGGER.failedToFindProtocolManager(ctx.channel().remoteAddress().toString(), ctx.channel().localAddress().toString(), protocolToUse, protocolMap.keySet().toString());
--- End diff --
Question, how sure/guarenteed is it that these objects will def be wont be null them selves, aka doing this logger wont cause an NPE.
ctx.channel().remoteAddress().toString()
ctx.channel().localAddress().toString()
is it worth null checking the dot train?
e.g. is it worth doing?
ctx.channel() == null ? null : ctx.channel().localAddress() == null ? null : ctx.channel().localAddress().toString
---