You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/07/08 14:05:31 UTC
[GitHub] [qpid-broker-j] dakirily opened a new pull request #100: QPID-8549 - [Broker-J] HP Fortify: Null de-reference
dakirily opened a new pull request #100:
URL: https://github.com/apache/qpid-broker-j/pull/100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] alex-rufous commented on a change in pull request #100: QPID-8549 - [Broker-J] HP Fortify: Null de-reference
Posted by GitBox <gi...@apache.org>.
alex-rufous commented on a change in pull request #100:
URL: https://github.com/apache/qpid-broker-j/pull/100#discussion_r669981358
##########
File path: broker-core/src/main/java/org/apache/qpid/server/queue/SortedQueueEntryList.java
##########
@@ -108,28 +108,31 @@ private void insert(final SortedQueueEntry entry)
}
entry.setParent(parent);
- if(entry.compareTo(parent) < 0)
+ if (parent != null)
Review comment:
I do not think that check for null is required here.
The "if" and "while loop" above makes sure that parent is not null.
I also think, that when parent is null, it makes more sense to fail by throwing an Exception as entry will not be inserted and lost...
I would like to suggest reverting of this particular change...
##########
File path: bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/BDBConfiguredObjectRecord.java
##########
@@ -111,7 +111,10 @@ public int hashCode()
@Override
public String toString()
{
- return "BDBConfiguredObjectRecord [id=" + _id + ", type=" + _type + ", name=" + (_attributes == null ? null : _attributes.get("name")) + ", parents=" + _parents + "]";
+ return "BDBConfiguredObjectRecord [id=" + String.valueOf(_id)
Review comment:
Though, the purpose of the PR JIRA is to fix null references, as this toString() is changed , I strongly suggest to replace concatenation with `String.format()`. The latter works better in toString and makes it easier to read.
##########
File path: broker-core/src/main/java/org/apache/qpid/server/transport/MultiVersionProtocolEngine.java
##########
@@ -464,13 +464,16 @@ public void received(QpidByteBuffer msg)
_broker.getEventLogger().message(new PortLogSubject(_port),
PortMessages.UNSUPPORTED_PROTOCOL_HEADER(Functions.str(headerBytes),
- supportedReplyVersion.toString()));
+ String.valueOf(supportedReplyVersion)));
- try (QpidByteBuffer supportedReplyBuf = QpidByteBuffer.allocateDirect(supportedReplyBytes.length))
+ if (supportedReplyBytes != null)
Review comment:
The broker is required by the AMQP specification to send a reply with a latest supported AMQP version.
Thus, if supportedReplyBytes is null, it is a defect.
I would like to suggest adding a check whether supportedReplyBytes is null. If it is null, we we can iterate via available creators and pickup the highest version among available and set supportedReplyBytes for the highest available protocol.
if there is no creator, that would mean that all protocols are disabled.... In that case the broker cannot support messaging... I am not sure what the broker behaviour should be in this case... It seems that broker is inconsistent state and possibly would need to bring down...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] dakirily commented on pull request #100: QPID-8549 - [Broker-J] HP Fortify: Null de-reference
Posted by GitBox <gi...@apache.org>.
dakirily commented on pull request #100:
URL: https://github.com/apache/qpid-broker-j/pull/100#issuecomment-881612281
In BDBConfiguredObjectRecord toString() was refactored to use String.format().
In SortedQueueEntryList null check for parent entry was changed to throw an exception. Would it be OK or would you prefer to leave to existing logic without any changes at all?
In MultiVersionProtocolEngine null check for supportedReplyBytes was extended with the logic suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] asfgit closed pull request #100: QPID-8549 - [Broker-J] HP Fortify: Null de-reference
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #100:
URL: https://github.com/apache/qpid-broker-j/pull/100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org