Incorrect length specification in loose encoding?

• Key: AMQ-718
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Hiram Chirino
• Reporter: Andrew Lusk
• Component/s: Transport
• Affects Version/s: 4.0, 4.0 RC2, 4.0 RC3
• Fix Version/s: 4.0

Description

Unless loose-encoded length is mean to mean something different from tight-encoded length, I believe that the length written to the wire for loosely-encoded OpenWire messages is off by 4. I imagine this hasn't been caught before because most clients synchronously read packets off the wire and so can ignore the length specification.

— activemq-core/src/main/java/org/apache/activemq/openwire/OpenWireFormat.java (revision 399408)
+++ activemq-core/src/main/java/org/apache/activemq/openwire/OpenWireFormat.java (working copy)
@@ -172,7 +172,7 @@
sequence = baos.toByteSequence();

if( !sizePrefixDisabled ) { - size = sequence.getLength()-4; + size = sequence.getLength(); ByteArrayPacket packet = new ByteArrayPacket(sequence); PacketData.writeIntBig(packet, size); }
@@ -253,7 +253,7 @@
if( !sizePrefixDisabled ) { looseOut.close(); ByteSequence sequence = baos.toByteSequence(); - dataOut.writeInt(sequence.getLength()-4); + dataOut.writeInt(sequence.getLength()); dataOut.write(sequence.getData(), sequence.getOffset(), sequence.getLength()); }

Comments

[James Strachan]

added versions

[Hiram Chirino]

Good find.. I think it's worth delaying the 4.0 release to fix this.

[Hiram Chirino]

oh. I think I may read this too quickly.

I just reviewed the code, and the size field is consistent in usage in both the loose and tight encoding. They size written is the number of bytes that will follow.

[Hiram Chirino]

As I see it both loose and tight encoding prefix the size of the playload.

So the total size of a command when it is encoding to bytes would be 4+1+size-of-payload which corresponds to the size-of-payload, type, and payload.

This is true for both loose and tight encoding. If I'm wrong please let me know.

[Andrew Lusk]

FWIW, my understanding is that size-of-payload includes the type byte, but not the size bytes. Correct? (see int size=1 on OpenWireFormat.java:223).

I think it may be right in one instance and wrong in another.

Using this code as an example, from OpenWireFormat.java:242:

DataOutputStream looseOut = dataOut;
ByteArrayOutputStream baos=null;

if( !sizePrefixDisabled ) { baos = new ByteArrayOutputStream(); looseOut = new DataOutputStream(baos); }

looseOut.writeByte(type);
dsm.looseMarshal(this, c, looseOut);

if( !sizePrefixDisabled ) { looseOut.close(); ByteSequence sequence = baos.toByteSequence(); dataOut.writeInt(sequence.getLength()-4); dataOut.write(sequence.getData(), sequence.getOffset(), sequence.getLength()); }

Nowhere was the size written into "baos" yet 4 is being subtracted from its size to put the size on the wire. In another location in the file (the other overload of marshal() :

ByteArrayOutputStream baos = new ByteArrayOutputStream();
DataOutputStream ds = new DataOutputStream(baos);
if( !sizePrefixDisabled ) { ds.writeInt(0); // we don't know the final size yet but write this here for now. }
ds.writeByte(type);
dsm.looseMarshal(this, c, ds);
ds.close();
sequence = baos.toByteSequence();

if( !sizePrefixDisabled ) { size = sequence.getLength()-4; ByteArrayPacket packet = new ByteArrayPacket(sequence); PacketData.writeIntBig(packet, size); }
}

In this case I think this code is right, since the size has been written in to that sequence.

I'll revert the second case I mentioned and run my tests again.

[Andrew Lusk]

OK, I've verified that my code still works with just the first case I mentioned (in the marshal(Object, DataOutputStream) overload) patched. The other case I believe is fine how it is.

[Hiram Chirino]

Hi Adrew.. this stuf is not easy grok.. glad your keeping us honest.

Your right, I stand corrected, the size field holds the size of the type+playload fields. For a sec there I was thinking it only held the size of the payload.

I now agree with your anyalisys that the original patch to line 253 is valid. And also found that the sizePrefixDisabled option was not being honorred in the tight encoding case for that method. This is the new patch what I will be applying shortly.

Index: /Users/chirino/sandbox/activemq-4-branch/activemq-core/src/main/java/org/apache/activemq/openwire/OpenWireFormat.java
===================================================================
— /Users/chirino/sandbox/activemq-4-branch/activemq-core/src/main/java/org/apache/activemq/openwire/OpenWireFormat.java (revision 399345)
+++ /Users/chirino/sandbox/activemq-4-branch/activemq-core/src/main/java/org/apache/activemq/openwire/OpenWireFormat.java (working copy)
@@ -233,11 +233,15 @@
BooleanStream bs = new BooleanStream();
size += dsm.tightMarshal1(this, c, bs);
size += bs.marshalledSize();

• dataOut.writeInt(size);
  +
  + if( !sizePrefixDisabled ) { + dataOut.writeInt(size); + }
  +
  dataOut.writeByte(type);
  bs.marshal(dataOut);
  dsm.tightMarshal2(this, c, dataOut, bs);
  +
  } else {
  DataOutputStream looseOut = dataOut;
  ByteArrayOutputStream baos=null;
  @@ -253,7 +257,7 @@
  if( !sizePrefixDisabled ) { looseOut.close(); ByteSequence sequence = baos.toByteSequence(); - dataOut.writeInt(sequence.getLength()-4); + dataOut.writeInt(sequence.getLength()); dataOut.write(sequence.getData(), sequence.getOffset(), sequence.getLength()); }

[Hiram Chirino]

Patch commited. Thanks Andrew!

[Hiram Chirino]

fixing in 4.0

[Hiram Chirino]

recut 4.0