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?

Answer 1

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;
}

Answer 2

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.