[Okta Webinar] Learn how to a build a cloud-first strategyRegister Now

x
  • Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 346
  • Last Modified:

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.
0
merimax
Asked:
merimax
  • 12
  • 7
  • 4
  • +1
1 Solution
 
t-maxCommented:
Have you recompiled your code in the 64 machine or did you just run your 32 compiled binary?
0
 
Infinity08Commented:
Instead of using unsigned long, try using use fixed width integer types (like uint32_t) everywhere where the code relies on a given width.
0
 
merimaxAuthor Commented:
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);
}
0
Concerto's Cloud Advisory Services

Want to avoid the missteps to gaining all the benefits of the cloud? Learn more about the different assessment options from our Cloud Advisory team.

 
Infinity08Commented:
>>       void (Blowfish::*p_ptrCryptFunc)(unsigned long *))

I still see unsigned long being used there ;)
0
 
merimaxAuthor Commented:
Yes, but even if I comment out that function call, I still core dump on p_ptrIn being out of bounds.
0
 
Infinity08Commented:
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).
0
 
merimaxAuthor Commented:
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;
    }
0
 
Infinity08Commented:
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 ?
0
 
merimaxAuthor Commented:
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);
}




0
 
Infinity08Commented:
>> unsigned int uintQueryLength ;

So what value does it have ?
0
 
merimaxAuthor Commented:
It just does a strlen() on the query text to get the value.
0
 
Infinity08Commented:
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.
0
 
sarabandeCommented:
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
0
 
Infinity08Commented:
>> 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.
0
 
sarabandeCommented:
that means it uses an int temporary for the expression?

Sara
0
 
Infinity08Commented:
>> 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.
0
 
sarabandeCommented:
but the 'overrun of p_ptrIn' as stated by the OP would fit to a non-compliancy in that point?

Sara
0
 
Infinity08Commented:
>>  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.
0
 
sarabandeCommented:
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
0
 
Infinity08Commented:
>> 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.
0
 
merimaxAuthor Commented:
It turns out this blowfish encoding was expecting BASE32 characters so by compiling it in 64 bit I was sending it BASE64 characters and that was what was blowing it up. The problem was not with the above code.
0
 
Infinity08Commented:
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 ?
0
 
Infinity08Commented:
>> 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).
0
 
merimaxAuthor Commented:
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.
0

Featured Post

Free Tool: SSL Checker

Scans your site and returns information about your SSL implementation and certificate. Helpful for debugging and validating your SSL configuration.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

  • 12
  • 7
  • 4
  • +1
Tackle projects and never again get stuck behind a technical roadblock.
Join Now