uint pack and unpack don't work for non-power of 2 byte_sizes

[s-snowden]

On a little-endian Windows machine, be_uint_fields with a size of 3 are not properly packed and unpacked. I believe this would be the case on other platforms too, and for any byte_size not handled by the standard struct pack stuff.

The followng snippet demonstrates the problem:

from infi.instruct.buffer import (Buffer, be_uint_field, le_uint_field, buffer_field,
                                  list_field, str_field)
from infi.instruct.buffer.macros import bytes_ref

class TestBufferLE(Buffer):
    three_byte_int = le_uint_field(where=bytes_ref[0:3])

class TestBufferBE(Buffer):
    three_byte_int = be_uint_field(where=bytes_ref[0:3])

tle = TestBufferLE()
tle.three_byte_int = 0x012345

print 'little-endian:'
print ':'.join(['%02x' % x for x in tle.pack()])

tle2 = TestBufferLE()
tle2.unpack(tle.pack())
print hex(tle2.three_byte_int)

print '*****************************************'

tbe = TestBufferBE()
tbe.three_byte_int = 0x012345

print 'big-endian:'
print ':'.join(['%02x' % x for x in tbe.pack()])
tbe2 = TestBufferBE()
tbe2.unpack(tbe.pack())
print hex(tbe2.three_byte_int)

The code above generates the following output:

little-endian
45:23:01
0x12345
*****************************************
big-endian
45:23:01
0x12345

It should generate:

little-endian:
45:23:01
0x12345
*****************************************
big-endian:
01:23:45
0x12345

The following patch corrects the problem:

--- serialize.py        2016-12-13 15:30:07 -0500
+++ serialize_fixed.py  2016-12-13 15:37:33 -0500
@@ -79,17 +79,24 @@

 def pack_int(value, **kwargs):
+    from sys import byteorder
     byte_size = kwargs_fractional_byte_size(kwargs)
     if byte_size in STRUCT_INT_PACKERS:
         return STRUCT_INT_PACKERS[byte_size](value, **kwargs)
     else:
         byte_size = kwargs.pop("byte_size", float(value.bit_length()) / 8)
-        return pack_bit_int(value, byte_size, **kwargs)
+        result = pack_bit_int(value, byte_size, **kwargs)
+        if byteorder != kwargs.get("endian"):
+            result.reverse()
+        return result

 def unpack_bit_int(buffer, byte_size, **kwargs):
     result = 0
-    l = reversed(buffer[0:byte_size]) if kwargs.get("endian", "big") else buffer[0:byte_size]
+    if kwargs.get("endian") != 'big':
+        l = reversed(buffer[0:byte_size])
+    else:
+        l = buffer[0:byte_size]
     for b in l:
         result *= 256
         result += b
@@ -299,3 +306,4 @@
         offset += item_len
         index += 1
     return result, offset
+

These are some nice modules you've created. I'd be happy to contribute some additional CDBs if you're interested.

[wiggin15]

Hi. Thanks for reporting the issue. The patch looks good.

• If kwargs is guraranteed to have endian we can use kwargs['endian'] instead of kwargs.get
• What happens if we use uint_field with the patch? If I am not mistaken, kwargs['endian'] will get native. In this case pack will reverse the order regardless of endianity, and unpack will reverse the order even on big-endian platforms.

Would you like to open a pull request with the patch so we can merge a commit that comes from you?

[amito]

However, it will be better if the fix will be a part of pack_bit_int and not pack_int. Also, thanks for the offer to contribute, we'll be glad for additional CDBs.

[s-snowden]

I agree that pack_bit_int is more appropriate. That patch was just to solve my immediate problem. I'll do the pull request with a more complete solution.

[s-snowden]

Sorry for the annoyance, but I haven't used git or github before. I cloned the repo, added some unit tests and my fix, and committed it locally, but when I went to push it back, I got a permission error. What did I do wrong?

The steps I took were:

1. git clone https://github.com/Infinidat/infi.instruct.git
2. Made my changes...
3. git commit
4. git push https://github.com/Infinidat/infi.instruct.git

Thanks!

[amito]

Hi again,

Any changes you make have to be pushed to your forked copy of infi.instruct, and later be accepted to our repo in a pull request. So, all work should be with your forked repo. As for you example, you need to change your 1st command to: git clone https://github.com/s-snowden/infi.instruct And your 4th command to: git push https://github.com/s-snowden/infi.instruct

When you're done, create a pull request (theres' a "new pull request" button in your repository web GUI).

Good luck! :)

[nworbttam]

Are there any updates to this issue? I'm experiencing an issue with this limitation currently. The PR was merged, but was backed out in this commit.

Is there a more graceful solution in mind @grzn ? Thanks!