Link to home
Start Free TrialLog in
Avatar of merimax
merimax

asked on

problems moving bit shifting from 32bit to 64bit linux

I have some code that works in 32 bit linux but when it is moved to 64 bit linux it core dumps. Here is the offending code...

void
Blowfish::
Crypt(const unsigned char * p_ptrIn,
      unsigned char * p_ptrOut,
      void (Blowfish::*p_ptrCryptFunc)(unsigned long *))
{
    unsigned long uintTemp[2];

    uintTemp[0] = (p_ptrIn[0] << 24) | (p_ptrIn[1] << 16) | (p_ptrIn[2] <<  8) |  p_ptrIn[3];
    uintTemp[1] = (p_ptrIn[4] << 24) | (p_ptrIn[5] << 16) | (p_ptrIn[6] <<  8) |  (size_t)p_ptrIn[7];
    (*this.*p_ptrCryptFunc)(&(uintTemp[0]));
    p_ptrOut[0] = (unsigned char)((size_t)(uintTemp[0] >> 24) & 0xff);
    p_ptrOut[1] = (unsigned char)((uintTemp[0] >> 16) & 0xff);
    p_ptrOut[2] = (unsigned char)((uintTemp[0] >>  8) & 0xff);
    p_ptrOut[3] = (unsigned char)(uintTemp[0] & 0xff);
    p_ptrOut[4] = (unsigned char)((uintTemp[1] >> 24) & 0xff);
    p_ptrOut[5] = (unsigned char)((uintTemp[1] >> 16) & 0xff);
    p_ptrOut[6] = (unsigned char)((uintTemp[1] >>  8) & 0xff);
    p_ptrOut[7] = (unsigned char)(uintTemp[1] & 0xff);
}

The p_ptrIn and p_ptrOut arrays are the ones that are getting overrun when I run in 64 bit but I don't understand why as they are not getting modified.

I am obviously missing something here. Increasing the size of uintTemp did not solve the problem.
Avatar of t-max
t-max
Flag of Israel image

Have you recompiled your code in the 64 machine or did you just run your 32 compiled binary?
Avatar of Infinity08
Instead of using unsigned long, try using use fixed width integer types (like uint32_t) everywhere where the code relies on a given width.
Avatar of merimax
merimax

ASKER

I tried changing the mask to uint32_t (see below) but it still core dumps on

uintTemp[0] = (p_ptrIn[0] << 24) | (p_ptrIn[1] << 16) | (p_ptrIn[2] <<  8) |  p_ptrIn[3];

Obviously I am missing something basic here. p_ptrIn[0] is a character so it is  8 bits wide. It is getting shifted over 24 bits into the 64 bit of the original uintTemp[0] (before I changed to uing32_t) . How can I be getting overflow on p_ptrIn?


void
Blowfish::
Crypt(const unsigned char * p_ptrIn,
      unsigned char * p_ptrOut,
      void (Blowfish::*p_ptrCryptFunc)(unsigned long *))
{
    uint32_t uintTemp[2];

    uintTemp[0] = (p_ptrIn[0] << 24) | (p_ptrIn[1] << 16) | (p_ptrIn[2] <<  8) |  p_ptrIn[3];
    uintTemp[1] = (p_ptrIn[4] << 24) | (p_ptrIn[5] << 16) | (p_ptrIn[6] <<  8) |  (size_t)p_ptrIn[7];
    (*this.*p_ptrCryptFunc)(&(uintTemp[0]));
    p_ptrOut[0] = (unsigned char)((size_t)(uintTemp[0] >> 24) & 0xff);
    p_ptrOut[1] = (unsigned char)((uintTemp[0] >> 16) & 0xff);
    p_ptrOut[2] = (unsigned char)((uintTemp[0] >>  8) & 0xff);
    p_ptrOut[3] = (unsigned char)(uintTemp[0] & 0xff);
    p_ptrOut[4] = (unsigned char)((uintTemp[1] >> 24) & 0xff);
    p_ptrOut[5] = (unsigned char)((uintTemp[1] >> 16) & 0xff);
    p_ptrOut[6] = (unsigned char)((uintTemp[1] >>  8) & 0xff);
    p_ptrOut[7] = (unsigned char)(uintTemp[1] & 0xff);
}
>>       void (Blowfish::*p_ptrCryptFunc)(unsigned long *))

I still see unsigned long being used there ;)
Avatar of merimax

ASKER

Yes, but even if I comment out that function call, I still core dump on p_ptrIn being out of bounds.
Then the problem is not caused in the code you showed. So, you'll have to show a bit more code (like the code that calls this function).
Avatar of merimax

ASKER

Here is where it is getting called...

    // Make sure the number of bytes is a multiple of 8 if we are decrypting
    //
    if ( p_blnEncrypt == false && *p_ptrLength % 8 != 0 )
        return false;

    Cipher(p_ptrKey, strlen(p_ptrKey));
    void (Blowfish::*ftnCryptFunc)(unsigned long *) = ( p_blnEncrypt )
        ? &Blowfish::Encipher
        : &Blowfish::Decipher;

    unsigned int  i;
    unsigned char uchrDataIn[8];
    unsigned char uchrDataOut[8];
    unsigned int  uintBytes = (*p_ptrLength) - 8;
    for (i=0; i<uintBytes; i+=8)
    {
        if ( p_ptrIn->Read(&uchrDataIn, 8) == false )
            return false;

        Crypt(uchrDataIn,
              uchrDataOut,
              ftnCryptFunc);

        if ( p_ptrOut->Write(&uchrDataOut, 8) == false )
            return false;
    }
What does this :

>>     unsigned int  uintBytes = (*p_ptrLength) - 8;

give you ?

Or in other words, how is p_ptrLength defined, and what value does *p_ptrLength hold ?
Avatar of merimax

ASKER

Ok....

We have...
unsigned int uintQueryLength ;

which gets passed to the below function...

clsCrypt.Crypt("Transfer.xml",
                             (unsigned char *)(p_ptrData->ptrQueryText),
                             &uintQueryLength,
                             false) == false )

which calls the following...

bool
Blowfish::
Crypt(const char * p_ptrKey,
      UTIL::Input * p_ptrIn,
      UTIL::Output * p_ptrOut,
      unsigned int * p_ptrLength,
      const bool p_blnEncrypt)
{
    //
    // Make sure the number of bytes is a multiple of 8 if we are decrypting
    //
    if ( p_blnEncrypt == false && *p_ptrLength % 8 != 0 )
        return false;

    Cipher(p_ptrKey, strlen(p_ptrKey));
    void (Blowfish::*ftnCryptFunc)(unsigned long *) = ( p_blnEncrypt )
        ? &Blowfish::Encipher
        : &Blowfish::Decipher;

    unsigned int  i;
    unsigned char uchrDataIn[8];
    unsigned char uchrDataOut[8];
    unsigned int  uintBytes = (*p_ptrLength) - 8;
    for (i=0; i<uintBytes; i+=8)
    {
        if ( p_ptrIn->Read(&uchrDataIn, 8) == false )
            return false;

        Crypt(uchrDataIn,
              uchrDataOut,
              ftnCryptFunc);

        if ( p_ptrOut->Write(&uchrDataOut, 8) == false )
            return false;
    }


which then core dumps when it gets to below code because p_ptrIn and  p_ptrOut are out of bounds...
void
Blowfish::
Crypt(const unsigned char * p_ptrIn,
      unsigned char * p_ptrOut,
      void (Blowfish::*p_ptrCryptFunc)(unsigned long *))
{
    uint32_t uintTemp[2];

    uintTemp[0] = (p_ptrIn[0] << 24) | (p_ptrIn[1] << 16) | (p_ptrIn[2] <<  8) |  p_ptrIn[3];
    uintTemp[1] = (p_ptrIn[4] << 24) | (p_ptrIn[5] << 16) | (p_ptrIn[6] <<  8) |  p_ptrIn[7];
    (*this.*p_ptrCryptFunc)(&(uintTemp[0]));
    p_ptrOut[0] = (unsigned char)((size_t)(uintTemp[0] >> 24) & 0xff);
    p_ptrOut[1] = (unsigned char)((uintTemp[0] >> 16) & 0xff);
    p_ptrOut[2] = (unsigned char)((uintTemp[0] >>  8) & 0xff);
    p_ptrOut[3] = (unsigned char)(uintTemp[0] & 0xff);
    p_ptrOut[4] = (unsigned char)((uintTemp[1] >> 24) & 0xff);
    p_ptrOut[5] = (unsigned char)((uintTemp[1] >> 16) & 0xff);
    p_ptrOut[6] = (unsigned char)((uintTemp[1] >>  8) & 0xff);
    p_ptrOut[7] = (unsigned char)(uintTemp[1] & 0xff);
}




>> unsigned int uintQueryLength ;

So what value does it have ?
Avatar of merimax

ASKER

It just does a strlen() on the query text to get the value.
Ok, it seems that wandering through the code like this won't give us an answer soon.

Why don't you run the code in the debugger, and it'll point you to exactly the line that has the issue, and will tell you the values that all variables have at that point.
I don't think that (p_ptrIn[0] << 24) is a valid expression for all compilers if p_ptrIn[0] is a char (though i wouldn't have expected it would crash).

same applies for all other bit shift operations which were shifting beyond 8-bit char boundary.

can you try to use for bitshift a temporary uint32_t array where you copied the values of p_ptrIn array before?

Sara
>> I don't think that (p_ptrIn[0] << 24) is a valid expression for all compilers if p_ptrIn[0] is a char

It's valid for all standards compliant compilers, due to integer promotion rules.
that means it uses an int temporary for the expression?

Sara
>> that means it uses an int temporary for the expression?

It means that the calculation is done after performing integer promotion, and in this case, it will promote p_ptrIn[0] to an int.
but the 'overrun of p_ptrIn' as stated by the OP would fit to a non-compliancy in that point?

Sara
>>  but the 'overrun of p_ptrIn' as stated by the OP would fit to a non-compliancy in that point?

A non-compliant compiler can do anything it wants, so anything is possible. In other words : I couldn't say if a non-compliant compiler would cause the described behavior, because with non-compliant compilers, all bets are off. Standards are there to avoid these issues (and integer promotion is a pretty basic part of C) though.
I compiled the following

    char c, d = 'a';
    c = (d << 24);

with vc9 and warning level 4 (the strongest).

it didn't give me a warning. shouldn't it have done so if integer promotion was done at the right side?

it gives a warning if i do

    int i = 17;
    c = i;

Sara
>> shouldn't it have done so if integer promotion was done at the right side?

The standard doesn't require warnings here. It's up to the compiler in which cases it gives a warning.

A compiler with high warning levels enabled might give a warning for this case (due to the potential of a value change), or it might not. gcc eg. gives a warning for this with -Wconversion.
ASKER CERTIFIED SOLUTION
Avatar of merimax
merimax

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Changing from 32bit to 64bit should have no impact on the choice of encoding between Base32 and Base64, because the two are not related.

Are you sure about your analysis ?
>> It turns out the problem was with the algorithm that worked on the string not the string where the compiler was telling me the array was out of bounds.

I suspect that you actually meant that the algorithm was interpreting the data as 64bit words rather than 32bit words. And that would have been resolved by the suggested changes (among other things : use the correct Blowfish crypt function by using a fixed width integer type rather than unsigned long).
Avatar of merimax

ASKER

It turns out the problem was with the algorithm that worked on the string not the string where the compiler was telling me the array was out of bounds.