Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So in my quest to make my AES128 code better and not to have memory leaks and the sort I came across something. Someone said "Unless you are forced to use C, you should never use malloc"

So I thought to myself, "Self, I will get rid of my malloc's". And in my Encrypt/Decrypt method I found this. Now i'm told that I should always finish with a delete[] on my byte array.. and I don't see how I would do it... maybe my methodology is wrong

here is how I call the method.

bool DESFireCard::AuthenticateOffline()
{
    AES128 aes;
    BYTE* RandB;
    BYTE buffer[33] = {0x00};
    BYTE lenRec = 0;
    BYTE IV[16] = {0x00};
    BYTE divkey[16];

    ByteUtil::StringToHex(key.DIV, divkey);
    aes.SetIV(IV);
    aes.SetKey(divkey);
    int state = rf_ISO14443_4_COS(APDU,2, buffer, &lenRec,2000);

    aes.DecryptData(&RandB, &buffer[1], 16);
    // LOTS more code HERE
    delete[] RandB;
    delete[] buffer; // <--crashes here
    delete[] IV;
    delete[] divkey;

    return true;
}

then my decrypt method (which is almost identical to the encrypt method)

void AES128::DecryptData(BYTE** outBlock, const BYTE* inBlock, size_t length)
{
    float blockSize = (float)(length/16);
    blockSize = ceilf(blockSize);
    size_t newLength = (size_t)(blockSize*16);
    BYTE* temp = new BYTE[newLength];//(BYTE*)malloc(newLength);
    BYTE* padd = new BYTE[newLength];//(BYTE*)malloc(newLength);
    memset(temp, 0, newLength);
    memcpy(padd, inBlock, length);
    DecryptBlock(temp, padd, IV);
    for (int i=1; i<blockSize; i++)
    {
        DecryptBlock(&temp[i*16], &padd[i*16], &temp[(i-1)*16]);
    }

    *outBlock = new BYTE[newLength]; //(BYTE*)malloc(newLength);
    memcpy((*outBlock), temp, newLength);

    delete[] temp;
    delete[] padd;
}
void AES128::DecryptBlock(BYTE* outBlock, const BYTE* inBlock, const BYTE* cipherBlock)
{
    BYTE temp[16] = {0x00};
    AES::XorBlock(temp, inBlock);

    BYTE expandedKey[176] = {0x00};
    BYTE* invExpandedKey;
    memcpy(expandedKey, Key, 16);
    Galois::expand_key(expandedKey);
    Galois::InvertExpandedKey(&invExpandedKey, expandedKey);
    AES::XorBlock(temp, invExpandedKey);
    AES::InvShiftRows(temp);
    AES::InvSubBytes(temp);
    for(int i=0x10; i<0xA0; i+=0x10) //change i<0xA0
    {
        AES::InvDoRound(temp, &invExpandedKey[i]);
    }
    AES::XorBlock(temp, &invExpandedKey[160]);
    AES::XorBlock(temp, cipherBlock);
    memcpy(outBlock, temp, 16);
}

so would I put a delete method in my AES128 class (see edit)? or do I put it at the end of me using it in the one method?

or should I put in a "outbuffer" in AES128 then in my destructor delete my outbuffer?

Sorry for a seemingyl newb question but C++ is not my strength (i hope it will be one day though :) )

EDIT1 ok I put in a few deletes and my program crashes saying something AccessViolationException

grar!! i hate that exception. I found out where it crashes, but i don't know why it would crash. Any tips on that? do i need to use delete on pre-allocated memories?

share|improve this question

closed as off topic by Corbin, svick, James Khoury, Brian Reichle, Glenn Rogers Feb 5 '13 at 10:17

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

1  
Your crash is due to trying to delete[] an array allocated on the stack. –  Richard J. Ross III Feb 4 '13 at 16:33
    
@RichardJ.RossIII I found that if I change all my arrays to BYTE* = new BYTE[size] that the problem goes away. Was that the correct move? –  Robert Snyder Feb 4 '13 at 16:37
1  
The rule is: use new[] to allocate and delete[] to deallocate. You can't deallocate a fixed-size array. You can only use delete[] to deallocate memory allocated with new[] as in your previous comment :) –  Morwenn Feb 4 '13 at 21:13
add comment

1 Answer

up vote 4 down vote accepted

Note:

x = new X;             =>  delete x;
// or
x = new X[Size];       =>  delete [] x;
// or
x = (X)malloc(exp);    =>  free(x);

These are you three ways to allocate memory.
Each has a specific way to be released. The de-allocation MUST match the allocation.

Variables that are not allocated dynamcially should not be deleted (ie if you don't use new then don't call delete/free on them).

char   Buffer[100] = "Stuff"; // Not dynamically allocated
                              // No call to new.
                              // Therefore no call to delete

So:

These deletes are broken:

delete[] buffer;
delete[] IV;
delete[] divkey;

All of these buffers are declared locally.

// No new so these should not be deleted.
BYTE buffer[33] = {0x00};
BYTE IV[16] = {0x00};
BYTE divkey[16];

The othere delete here is done correctly.
But the design is bad for C++

delete[] RandB;  // This is OK as you allocated this with new
                 // though you hide the fact. and don't make the ownership
                 // very noticable.

The trouble here is the code is not exception safe (probably not a big deal for this code). But you should learn to write your code in a way that is exception safe for future C++.

A better idea would have been to use a dynamically sizeable container:

std::vector<BYTE>    RandB;

// Then change the function header:
// Notice we are passing by reference
void AES128::DecryptData(std::vector<BYTE>& outBlock, const BYTE* inBlock, size_t length)

// Then you can resize it when you know the size:
outBlock.resize(newLength);        //(BYTE*)malloc(newLength);
memcpy(&outBlock[0], temp, newLength);

Notice that container contains the new/delete inside its constructor/destructor so they will always be called (even if there is an exception).

Similarly:

BYTE* temp = new BYTE[newLength];//(BYTE*)malloc(newLength);
BYTE* padd = new BYTE[newLength];//(BYTE*)malloc(newLength);

// should replace with:
std::vector<BYTE> temp(newLength);
std::vector<BYTE> padd(newLength);

// now there is no need to delete the objects.
// it is done automatically.

//This does result in a small change in usage:

memset(temp, 0, newLength);
memcpy(padd, inBlock, length);
// Change too
memset(&temp[0], 0, newLength);
memcpy(&padd[0], inBlock, length);
share|improve this answer
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.