Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 2.6.3
    • 2.6.4, 2.7.0
    • None
    • None

    Attachments

      Issue Links

        Activity

          githubbot ASF GitHub Bot added a comment -

          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 <jbertram@...>
          Date: 2018-09-26T19:27:14Z

          ARTEMIS-2098 potential NPE when decoding protocol


          githubbot ASF GitHub Bot added a comment - 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 <jbertram@...> Date: 2018-09-26T19:27:14Z ARTEMIS-2098 potential NPE when decoding protocol
          githubbot ASF GitHub Bot added a comment -

          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

          githubbot ASF GitHub Bot added a comment - 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
          githubbot ASF GitHub Bot added a comment -

          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.

          githubbot ASF GitHub Bot added a comment - 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.

          Commit c72bf53cb178d2d9d310f3f369c0cbdb22e6f50c in activemq-artemis's branch refs/heads/master from jbertram
          [ https://git-wip-us.apache.org/repos/asf?p=activemq-artemis.git;h=c72bf53 ]

          ARTEMIS-2098 potential NPE when decoding protocol

          jira-bot ASF subversion and git services added a comment - Commit c72bf53cb178d2d9d310f3f369c0cbdb22e6f50c in activemq-artemis's branch refs/heads/master from jbertram [ https://git-wip-us.apache.org/repos/asf?p=activemq-artemis.git;h=c72bf53 ] ARTEMIS-2098 potential NPE when decoding protocol
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/activemq-artemis/pull/2334

          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/activemq-artemis/pull/2334

          Commit 19998d310f143ad5370683db42c63f64b43ee338 in activemq-artemis's branch refs/heads/2.6.x from jbertram
          [ https://git-wip-us.apache.org/repos/asf?p=activemq-artemis.git;h=19998d3 ]

          ARTEMIS-2098 potential NPE when decoding protocol

          (cherry picked from commit c72bf53cb178d2d9d310f3f369c0cbdb22e6f50c)

          jira-bot ASF subversion and git services added a comment - Commit 19998d310f143ad5370683db42c63f64b43ee338 in activemq-artemis's branch refs/heads/2.6.x from jbertram [ https://git-wip-us.apache.org/repos/asf?p=activemq-artemis.git;h=19998d3 ] ARTEMIS-2098 potential NPE when decoding protocol (cherry picked from commit c72bf53cb178d2d9d310f3f369c0cbdb22e6f50c)

          People

            Unassigned Unassigned
            jbertram Justin Bertram
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: