Memory leak using Malloc/Free in C

Question

I've been reading up on the use of pointers, and allocating memory for embedded projects. I must admit, that i perhaps don't understand it fully, as i can't seem to figure where my problem lies.

My two functions are supposed to take 4 float values, and return 16 bytes, that represent these, in order to transfer them through SPI. It works great, but only for a minute, before the program crashes and my SPI and I2C dies, lol.

Here are the functions:

/*Function that wraps a float value, by allocating memory and casting pointers. 
Returns 4 bytes that represents input float value f.*/
typedef char byte;

byte* floatToByteArray(float f)
{
    byte* ret = malloc(4 * sizeof(byte));
    unsigned int asInt = *((int*)&f);
    
    int i;
    for (i = 0; i < 4; i++) {
        ret[i] = (asInt >> 8 * i) & 0xFF;
    }

    return ret;
    memset(ret, 0, 4 * sizeof(byte)); //Clear allocated memory, to avoid taking all memory
    free(ret);
}

/*Takes a list of 4 quaternions, and wraps every quaternion in 4 bytes.
Returns a 16 element byte list for SPI transfer, that effectively contains the 4 quaternions*/
void wrap_quaternions(float Quaternion[4], int8_t *buff)
{
    uint8_t m;
    uint8_t n;
    uint8_t k = 0; 
    
    for (m = 0; m < 4; m++)
    {   
        for (n = 0; n < 4; n++)
        {
            byte* asBytes = floatToByteArray(Quaternion[m]);
            buff[n+4*k] = asBytes[n];
        }   
        k++;
    }
}

The error message i receive after is the following, in the disassembly window of Atmel Studio

Atmel studio screenshot

Answer 1

Gerhardh is correct, return prevent the memory from being released.

If you need to return 4 bytes, you might check if your environment can return a uint32_t or something like that.

Answer 2

As already mentioned, the lines below return ret; are never executed. And anyway if you want to return allocated memory in a function (what is fine) you can't free it in the function itself but it has to be freed by the caller when it isn't needed anymore. So your calling function should look like

/*Takes a list of 4 quaternions, and wraps every quaternion in 4 bytes.
Returns a 16 element byte list for SPI transfer, that effectively contains the 4 quaternions*/
void wrap_quaternions(float Quaternion[4], int8_t *buff)
{
    uint8_t m;
    uint8_t n;
    uint8_t k = 0; 
    
    for (m = 0; m < 4; m++)
    {   
        byte* asBytes = floatToByteArray(Quaternion[m]); // no need it to call for every n
        for (n = 0; n < 4; n++)
        {
             buff[n+4*k] = asBytes[n];
        }
        free(asBytes);  // asBytes is no longer needed and can be free()d 
        k++; 
    }
}

Answer 3

You might drop all the dynamic memory allocation completely.

void floatToByteArray(float f, byte buf[4])
{
    memcpy(buf, &f, sizeof(f));
}

void wrap_quaternions(float Quaternion[4], int8_t *buff)
{   
    for (int i = 0; i < 4; i++)
    {   
        floatToByteArray(Quaternion[i], &buf[4*i]);
    }
}

With this approach you do not need to care about freeing allocated memory after use. It is also much more efficient because dynamic memory allocation is rather expensive.

Answer 4

regarding:

buff[n+4*k] = asBytes[n];

This results in:

buff[0] << asBytes[0]  // from first call to `byte* floatToByteArray(float f)`
buff[4] << asBytes[1]  // from second call to `byte* floatToByteArray(float f)`

buff[8] << asBytes[2]  // from third call to `byte* floatToByteArray(float f)`

buff[12] << asBytes[3] // from forth call to `byte* floatToByteArray(float f)`

most of the above problem can be fixed by using memcpy() to copy the 4 bytes from asBytes[] to buff[] similar to:

memcpy( &buff[ n*4 ], asBytes, 4 );

Of course, there is also the consideration: Is the length of a float, on your hardware/compiler actually 4 bytes.

'magic' numbers are numbers with no basis. 'magic' numbers make the code much more difficult to understand, debug, etc. I.E. 4. Suggest using something like: length = sizeof( float ); then using length everywhere that 4 is currently being used, except for the number of entries in the Quaternion[] array. for that 'magic' number, strongly suggest the statement: #define arraySize 4 be early in your code. Then using arraySize each time the code references the number of elements in the array