[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
GitHub user stanlyDoge opened a pull request:

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

    ARTEMIS-1801 removing null-unchecked dereferences

    There were some cases where value was checked for null and later directly dereferenced without check. I added checks. I am not sure about "default behaviour" tho. Can anybody check?

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/stanlyDoge/activemq-artemis ARTEMIS-1801

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2010.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 #2010
   
----
commit 8f393828473c75321b28a4fe63f5f08fb7006bbe
Author: Stanislav Knot <sknot@...>
Date:   2018-04-11T06:43:07Z

    ARTEMIS-1801 removing null-unchecked dereferences

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
Github user stanlyDoge commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180649567
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
    @@ -2769,7 +2769,7 @@ public Queue createQueue(final AddressInfo addrInfo,
              throw ActiveMQMessageBundle.BUNDLE.invalidRoutingTypeForAddress(rt, info.getName().toString(), info.getRoutingTypes());
           }
     
    -      final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
    +      final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo == null ? RoutingType.ANYCAST :addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
    --- End diff --
   
    What should be default routing type?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user jdanekrh commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180652792
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
    @@ -2769,7 +2769,7 @@ public Queue createQueue(final AddressInfo addrInfo,
              throw ActiveMQMessageBundle.BUNDLE.invalidRoutingTypeForAddress(rt, info.getName().toString(), info.getRoutingTypes());
           }
     
    -      final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
    +      final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo == null ? RoutingType.ANYCAST :addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
    --- End diff --
   
    About 20 lines above this one is
   
        RoutingType rt = (routingType == null ? ActiveMQDefaultConfiguration.getDefaultRoutingType() : routingType);
   
    [here](https://github.com/apache/activemq-artemis/blob/fa3e37fdb91c285bcfe87cab3d771dfe80019e18/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java#L2754) and [here](https://github.com/apache/activemq-artemis/blob/fa3e37fdb91c285bcfe87cab3d771dfe80019e18/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java#L2865)
   
    So maybe that one?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user stanlyDoge commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180654379
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
    @@ -2769,7 +2769,7 @@ public Queue createQueue(final AddressInfo addrInfo,
              throw ActiveMQMessageBundle.BUNDLE.invalidRoutingTypeForAddress(rt, info.getName().toString(), info.getRoutingTypes());
           }
     
    -      final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
    +      final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo == null ? RoutingType.ANYCAST :addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
    --- End diff --
   
    @jdanekrh Good catch!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180858361
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java ---
    @@ -380,7 +380,7 @@ public Packet sendBlocking(final Packet packet,
     
                 connection.getTransportConnection().write(buffer, false, false);
     
    -            long toWait = connection.getBlockingCallTimeout();
    --- End diff --
   
    Looking at the code, I can't see how connection will ever be null here..  I think you should remove this one...
   
    For instance... the very same connection was used before within a lock.. what would issue a NPE a lot earlier than here.
   
    I don't see how connection could ever be null.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180858486
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java ---
    @@ -511,22 +511,24 @@ public void close() {
        public void transferConnection(final CoreRemotingConnection newConnection) {
           // Needs to synchronize on the connection to make sure no packets from
           // the old connection get processed after transfer has occurred
    -      synchronized (connection.getTransferLock()) {
    -         connection.removeChannel(id);
    +      if (connection != null) {
    --- End diff --
   
    same thing here.. how connection would be null?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180859106
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java ---
    @@ -205,7 +205,7 @@ public PageSubscriptionCounter getCounter() {
         */
        @Override
        public boolean reloadPageCompletion(PagePosition position) throws Exception {
    -      if (!pageStore.checkPageFileExists((int)position.getPageNr())) {
    +      if (pageStore != null && !pageStore.checkPageFileExists((int)position.getPageNr())) {
    --- End diff --
   
    pageStore is final.. it will never be null


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180859389
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2712,6 +2712,10 @@ private Message makeCopy(final MessageReference ref, final boolean expiry) throw
        private Message makeCopy(final MessageReference ref,
                                 final boolean expiry,
                                 final boolean copyOriginalHeaders) throws Exception {
    +      if (ref == null) {
    +         return null;
    --- End diff --
   
    I would actually throw an Exception.. you're not supposed to make a copy of a ref == null.
   
    it's a valid assertion, but I would actually make it throw NullPointerException  here. (Or a chosen exception).


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180859972
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2727,7 +2731,7 @@ private Message makeCopy(final MessageReference ref,
           Message copy = message.copy(newID);
     
           if (copyOriginalHeaders) {
    -         copy.referenceOriginalMessage(message, ref != null ? ref.getQueue().getName().toString() : null);
    +         copy.referenceOriginalMessage(message, ref.getQueue().getName().toString());
    --- End diff --
   
    keep it ! :) nice one!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user stanlyDoge commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180971622
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java ---
    @@ -511,22 +511,24 @@ public void close() {
        public void transferConnection(final CoreRemotingConnection newConnection) {
           // Needs to synchronize on the connection to make sure no packets from
           // the old connection get processed after transfer has occurred
    -      synchronized (connection.getTransferLock()) {
    -         connection.removeChannel(id);
    +      if (connection != null) {
    --- End diff --
   
    Well, it is null-checked few lines bellow so it confued me.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user stanlyDoge commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r180976916
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2727,7 +2731,7 @@ private Message makeCopy(final MessageReference ref,
           Message copy = message.copy(newID);
     
           if (copyOriginalHeaders) {
    -         copy.referenceOriginalMessage(message, ref != null ? ref.getQueue().getName().toString() : null);
    +         copy.referenceOriginalMessage(message, ref.getQueue().getName().toString());
    --- End diff --
   
    @clebertsuconic thank you :) Can you please check again? Especially null reference logging.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r181386656
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2712,6 +2713,11 @@ private Message makeCopy(final MessageReference ref, final boolean expiry) throw
        private Message makeCopy(final MessageReference ref,
                                 final boolean expiry,
                                 final boolean copyOriginalHeaders) throws Exception {
    +      if (ref == null) {
    +         ActiveMQServerLogger.LOGGER.nullRefMessage();
    +         throw new ActiveMQNullRefException("Reference to message is null");
    --- End diff --
   
    Was this an accident? WEren't you supposed to use NULL_REF.createException here?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2010: ARTEMIS-1801 removing null-unchecked dereferen...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user stanlyDoge commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2010
 
    @clebertsuconic polished :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r183418125
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2712,6 +2714,11 @@ private Message makeCopy(final MessageReference ref, final boolean expiry) throw
        private Message makeCopy(final MessageReference ref,
                                 final boolean expiry,
                                 final boolean copyOriginalHeaders) throws Exception {
    +      if (ref == null) {
    +         ActiveMQServerLogger.LOGGER.nullRefMessage();
    +         NULL_REF.createException("Reference to message is null");
    --- End diff --
   
    didn't you miss throw NULL_REF here?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r184572931
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2712,6 +2714,11 @@ private Message makeCopy(final MessageReference ref, final boolean expiry) throw
        private Message makeCopy(final MessageReference ref,
                                 final boolean expiry,
                                 final boolean copyOriginalHeaders) throws Exception {
    +      if (ref == null) {
    +         ActiveMQServerLogger.LOGGER.nullRefMessage();
    +         NULL_REF.createException("Reference to message is null");
    --- End diff --
   
    @stanlyDoge what was your intention here? this last expression here is pretty much dead code.. you are creating an exception and doing nothing with it... the previous logging would work.. but the exception here is dead code.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2010: ARTEMIS-1801 removing null-unchecked dereferen...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2010
 
    there's an open statement with a question I asked you.. and can you fix this easy to fix conflict?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user stanlyDoge commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r184600627
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2712,6 +2714,11 @@ private Message makeCopy(final MessageReference ref, final boolean expiry) throw
        private Message makeCopy(final MessageReference ref,
                                 final boolean expiry,
                                 final boolean copyOriginalHeaders) throws Exception {
    +      if (ref == null) {
    +         ActiveMQServerLogger.LOGGER.nullRefMessage();
    +         NULL_REF.createException("Reference to message is null");
    --- End diff --
   
    Yeah, i forgot to throw that. I am throwing ActiveMQNullRefException, which under the hood does super(ActiveMQExceptionType.NULL_REF). Is that ok?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user stanlyDoge commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r184601120
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2712,6 +2713,11 @@ private Message makeCopy(final MessageReference ref, final boolean expiry) throw
        private Message makeCopy(final MessageReference ref,
                                 final boolean expiry,
                                 final boolean copyOriginalHeaders) throws Exception {
    +      if (ref == null) {
    +         ActiveMQServerLogger.LOGGER.nullRefMessage();
    +         throw new ActiveMQNullRefException("Reference to message is null");
    --- End diff --
   
    ActiveMQNullRefException calls super(ActiveMQExceptionType.NULL_REF), which as I understand invokes NULL_REF.createException.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2010#discussion_r184701274
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java ---
    @@ -2712,6 +2713,11 @@ private Message makeCopy(final MessageReference ref, final boolean expiry) throw
        private Message makeCopy(final MessageReference ref,
                                 final boolean expiry,
                                 final boolean copyOriginalHeaders) throws Exception {
    +      if (ref == null) {
    +         ActiveMQServerLogger.LOGGER.nullRefMessage();
    +         throw new ActiveMQNullRefException("Reference to message is null");
    --- End diff --
   
    The method I saw didn't have a throw.. I'm not sure what happened to github diffs here.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user asfgit closed the pull request at:

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


---