Reversing a bit packing algorithm

Question

I'm trying to reverse an algorithm I have that packs a bunch of unsigned shorts into memory. I attempted to reverse it, and I get correct numbers back 50% - 75% of the time, so I'm not sure what I'm doing wrong.

This is the algorithm to pack the numbers into memory:

BYTE packNumber(BYTE bStart, WORD value, BYTE * buffer, DWORD * counter)
{
    value = (value<<(6-bStart));
    *(buffer + *counter) |= (BYTE)(value>>8);
    *(buffer + *counter+1) |= (BYTE)value;

    bStart = (bStart+2)%8;

    if (bStart)
        *counter+= 1;
    else
        *counter+= 2;

    return bStart;
}

This is called several times in a row, passing the returned bStart into the next call, until all the numbers have been packed into memory.

This is my attempt at reversing it:

BYTE unpackNumber(BYTE bStart, WORD *value, BYTE * buffer, DWORD * counter)
{
    *value= 0;

    *value|= *(buffer + *counter);
    *value= *value<< 8;
    *value|= *(buffer + *counter+1);

    *wVal = (*value>>(6-bStart));

    bStart = (bStart+2)%8;

    if (bStart)
        *counter+= 1;
    else
        *counter+= 2;

    return bStart;
}

I know I'm doing something right, since I get back a bunch of correct data, though depending on the numbers I wrote into memory, anything from every forth to every second number read in is wrong.

Any ideas what I'm doing wrong here?

could you explain in english your packing algo? Even as a mathematical formula (for readability) ie: value = (x* 8) | yz << xyz — Adrian, Feb 10 '12 at 18:58
Even after my answer below, I'm not really sure exactly how you are trying to compact stuff, just analyzing the code. — Michael Dorgan, Feb 10 '12 at 19:01
@Adrian It looks like it takes the 10 low bits out of every 16 bit word, packing 4 of them back to back in 5 bytes (40 bits). — Joachim Isaksson, Feb 10 '12 at 19:21
@Joachim, Yeah the algorithm is for storing a set of 10bit numbers into the most compact memory footprint it can. — Joel Barsotti, Feb 10 '12 at 21:12

Joachim Isaksson · Accepted Answer · 2012-02-10T20:14:31.843

3

Just looking at the code quickly, it looks like it should be working as WORD is unsigned.

I'd almost bet that it's not, and your shifts end up being signed (and thus the high bits shifted in will not be zero but copies of the sign bit)

Edit: Also, since you want 10 bits out, you should probably remove the possible extra high bits with *wVal &= 0x03ff.

This seems to work with WORD being unsigned short;

BYTE unpackNumber(BYTE bStart, WORD *value, BYTE * buffer, DWORD * counter)
{
    *value= 0;

    *value|= *(buffer + *counter);
    *value= *value<< 8;
    *value|= *(buffer + *counter+1);

    *value = (*value>>(6-bStart)) & 0x3ff; // <-- remove extraneous bits

    bStart = (bStart+2)%8;

    if (bStart)
        *counter+= 1;
    else
        *counter+= 2;

    return bStart;
}

edited Feb 10 '12 at 20:14

answered Feb 10 '12 at 19:06

Joachim Isaksson

176,943
25
281
294

An excellent guess as well - but this doesn't explain why it sometimes works. – Michael Dorgan Feb 10 '12 at 19:08
@MichaelDorgan It would work on packed words where bStart=6 or the high bit is 0, and fail on packed words where bstart!=6 and the high bit is 1. Not random, but 1/2-1/4 sounds about right frequency. – Joachim Isaksson Feb 10 '12 at 19:11
Good point. Without feedback on his function's input though, we must keep guessing. assert() for the love of god - err binary! – Michael Dorgan Feb 10 '12 at 19:15
I thought a logical shift would always rotate in 0s. How to I do an unsigned shift? – Ash Feb 10 '12 at 19:18
@ash An unsigned type will use an unsigned shift, a signed one will use a signed shift. In other words, use an unsigned datatype. – Joachim Isaksson Feb 10 '12 at 19:22
@Ash signed short a = 0x8000; unsigned short b = 0x8000; a>>=1; // (11000000 00000000) b>>=1; // (01000000 00000000) – Joachim Isaksson Feb 10 '12 at 19:27
I casted all of the hard coded numbers I'm shifting by to BYTE (*wVal = *wVal << (BYTE)8;) and it made no difference in the results. – Ash Feb 10 '12 at 19:28
It's the source datatype that matters, in other words *wVal needs to be unsigned when you shift it. Something like *wVal = ((unsigned WORD)(*wVal))>>(6-bStart); – Joachim Isaksson Feb 10 '12 at 19:35
WORD is just a typedef of an unsigned short. I did try your suggestion though. No success =( – Ash Feb 10 '12 at 19:47
@Ash Did you try removing the extra bits too as my edit added? – Joachim Isaksson Feb 10 '12 at 19:52

score 0 · Answer 2 · answered Feb 10 '12 at 18:58

0

Ouchy, your C code hurts my head a bit. Are you asserting any of your entrance conditions? The way your write out to RAM is just scary without checks. Also, if bStart is ever greater than 6, your shift is undefined. My guess is that bStart > 6 is your problem. BTW, this is not at all C++ code, it is straight C.

answered Feb 10 '12 at 18:58

Michael Dorgan

12,453
3
31
61

Yeah the code hurts my head too. I'm making sure all the data the function gets is valid. Also because of the line bStart = (bStart+2)%8; bStart rotates from 0,2,4,6 – Ash Feb 10 '12 at 19:00
Or 1,3,5,7 depending on the intial entrance value. Also, first time through, bstart could be > 6. I really would add some sort of assert() to bstart. – Michael Dorgan Feb 10 '12 at 19:03
The first time through bStart is 0. I agree that the code could definitely use some safety measures, but I didn't write the original algorithm and right now I'm just trying to get it to work. – Ash Feb 10 '12 at 19:52

Reversing a bit packing algorithm

2 Answers2