[
https://issues.apache.org/jira/browse/QPID-7379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15493794#comment-15493794
]
Lorenz Quack edited comment on QPID-7379 at 9/26/16 12:08 PM:
--------------------------------------------------------------
Here are my review comments:
* The Content-Disposition should also use the extended "filename*" syntax for
UTF-8 encoded filenames. (See {{AbstractQueue$MessageContent}})
* {{AbstractVirtualHost#importMessageStore}} can be used for a DoS attack by
crafting a "store" containing just 5 bytes: "0x00 MAX_INT" which will allocate
a byte array of 2 GB which potentially exhaust the broker's heap bringing down
the broker with an OOM Error. Maybe we limit the version string length to 1
byte? In that case the arbitrary {{50}} in {{data.mark(50)}} could be replaced
with an accurate upper bound on the reads like {{1+1+256}}. I guess a similar
DoS can be performed at many point within the serialisation stream. I think we
should guard against this by disallowing deserialisation of messages that
exceed {{qpid.max_message_size}}.
* I believe the {{0}} that is expected at the beginning of
{{AbstractVirtualHost#importMessageStore}} is actually a
{{serializer.v1.RecordType#VERSION}}
* It might be nicer to just throw the data stream at all serializers that are
available through the QpidServiceLoader and have them handle or reject the data
instead of putting knowledge of the serialisation format into the
{{AbstractVirtualHost}}
* extraction and import of message store happens in the http thread rather than
the config thread meaning it could race with other operations (including
another import/export, reactivation of the VHost, etc.).
* extract / import is not a symmetric nomenclature. I would suggest extract /
inject or export / import. I would vote for the latter because the former
sounds more like a live operation with potentially non-empty stores.
* Indentation of {{Deserializer#readBytes}} and {{RecordType}} is a bit messy.
* I think the arithmatic in {{DTXRecord#getData}} is wrong when allocating the
bytes array. The line {{(24 * _enqueues.length + _dequeues.length)}} should be
{{(24 * (_enqueues.length + _dequeues.length))}}
* minor nitpick, but I would prefer the lines {{_xid.getBranchId().length}} and
{{_xid.getGlobalId().length}} in the calculation of the length of the byte
array to be swtiched around to match the order they will be written.
* Why do you use arrays instead of direct memory? This seems especially odd for
potentially large chunks like the message content. Also, is it necessary to
make an private copy the data in the {{MessageRecord}}? Could you not just copy
it from the {{storedMessage}} in {{MessageRecord.getData()}}?
* The names {{MessageStoreSerializer_v1#deserializeDtx}} and
{{MessageStoreSerializer_v1#serializeDistributedTransactions}} should be made
symmetric
was (Author: lorenz.quack):
WIP Review:
* The Content-Disposition should also use the extended "filename*" syntax for
UTF-8 encoded filenames. (See {{AbstractQueue$MessageContent}})
* {{AbstractVirtualHost#importMessageStore}} can be used for a DoS attack by
crafting a "store" containing just 5 bytes: "0x00 MAX_INT" which will allocate
a byte array of 2 GB which potentially exhaust the broker's heap bringing down
the broker with an OOM Error. Maybe we limit the version string length to 1
byte? In that case the arbitrary {{50}} in {{data.mark(50)}} could be replaced
with an accurate upper bound on the reads like {{1+1+256}}. TODO: check whether
other parts of the deserializer are equally vulnerable.
* I believe the {{0}} that is expected at the beginning of
{{AbstractVirtualHost#importMessageStore}} is actually a
{{serializer.v1.RecordType#VERSION}}
* It might be nicer to just throw the data stream at all serializers that are
available through the QpidServiceLoader and have them handle or reject the data
instead of putting knowledge of the serialisation format into the
{{AbstractVirtualHost}}
> [Java Broker] Provide a mechanism to extract messages from a vhost message
> store and replay them into a new vhost
> -----------------------------------------------------------------------------------------------------------------
>
> Key: QPID-7379
> URL: https://issues.apache.org/jira/browse/QPID-7379
> Project: Qpid
> Issue Type: Improvement
> Components: Java Broker
> Reporter: Rob Godfrey
> Assignee: Rob Godfrey
> Fix For: qpid-java-6.1
>
>
> QPID-7359 provided operations to extract the config from a virtual host, but
> there are not currently any mechanisms to extract the contents of the message
> store and replay that into a new vhost. We should add this feature to (for
> example) allow people to migrate their data from one vhost type to another
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
