ruby-amqp / amq-protocol

AMQP 0.9.1 protocol serialization and deserialization implementation for Ruby (2.0+)
http://groups.google.com/group/ruby-amqp
MIT License
48 stars 31 forks source link

frame type 's' (16 signed ints) isn't evaluated correctly. #36

Closed ghost closed 10 years ago

ghost commented 10 years ago

When parsing 16bit signed frames around the AMQ::Pack module incorrectly converts bytes:

specifically:

...
  INT16  = "c".freeze
...
  def self.unpack_int16_big_endian(data)
    data = data.bytes.to_a.reverse.map(&:chr).join
    data.unpack(INT16)
  end

data.unpack(INT16) // INT16 'c' doesn't unpack the data correctly (only 8 bit) - from the ruby string.unpack docs:

    #  Integer      |         |
    #  Directive    | Returns | Meaning
    #  -----------------------------------------------------------------
    #     C         | Integer | 8-bit unsigned (unsigned char)
    #     S         | Integer | 16-bit unsigned, native endian (uint16_t)
    #     L         | Integer | 32-bit unsigned, native endian (uint32_t)
    #     Q         | Integer | 64-bit unsigned, native endian (uint64_t)
    #               |         |
    #     c         | Integer | 8-bit signed (signed char)
    #     s         | Integer | 16-bit signed, native endian (int16_t)
    #     l         | Integer | 32-bit signed, native endian (int32_t)
    #     q         | Integer | 64-bit signed, native endian (int64_t)

I verified and tested this and replacing the problematic method with data.unpack('s') which resolved the issue.

michaelklishin commented 10 years ago

@BenjaminConlan s is native endian, we need network byte order (big endian). This works on your OS/CPU but won't on every combination.

So we need to do the same thing we do for some other data types, have two different implementations based on platform's endianness. Feel free to submit a pull request with tests.

ghost commented 10 years ago

Right you are, I'm not actually a ruby developer so don't think I'm the guy for the task of writing something like you suggest, but I'll be sure to forward this issue onto one of our more competent ruby developers whom will hopefully be able to resolve this and write the appropriate unit tests.

I'll just append some notes for implementations sake.

  n         | Integer | 16-bit unsigned, network (big-endian) byte order

from the aforementioned ruby documentation. and http://stackoverflow.com/questions/4886994/reading-a-binary-16-bit-signed-big-endian-integer-in-ruby (value & ~(1 << 15)) - (value & (1 << 15)).

michaelklishin commented 10 years ago

Fixed by #38.