changing from malloc to new in C++ [closed]

Question

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?

Answer 1

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