Do I really need malloc?

Question

I understand that malloc is used to dynamically allocate memory. In my code, I have the following function that I sometimes call:

int memory_get_log(unsigned char day, unsigned char date, unsigned char month){

    char fileName[11];
    unsigned long readItems, itemsToRead;
    F_FILE *file;

    sprintf(fileName, "%s_%u%u%u%s", "LOG", day, date, month, ".bin");

    file = f_open(fileName , "r");

    itemsToRead = f_filelength( fileName );

    //unsigned char *fileData = (unsigned char *) malloc(itemsToRead);
    unsigned char fileData[itemsToRead]; //here I am not using malloc

    readItems = f_read(fileData, 1, itemsToRead, file);

    transmit_data(fileData, itemsToRead);

    f_close(file);

    return 0;
}

As you may see, the number of items I read from the file can be different each time. The line unsigned char fileData[itemsToRead]; is used to read these variable sized files. I can see that I am allocating memory dynamically in some way. This function works fine. Do I really need to use malloc here? Is there anything wrong with the way I am declaring this array?

Answer 1

TL;DR

If you don't know what you're doing, use malloc or a fixed size array in all situations. VLA:s are not necessary at all. And do note that VLA:s cannot be static nor global.

Do I really need to use malloc here?

Yes. You're reading a file. They are typically way bigger than what's suitable for a VLA. They should only be used for small arrays. If at all.

Long version

Is there anything wrong with the way I am declaring this array?

It depends. VLA:s was removed as a mandatory component from C11, so strictly speaking, you are using compiler extensions, thus reducing the portability. In the future, VLA:s might (The chance is probably extremely low) get removed from your compiler. Maybe you also want to recompile the code on a compiler without support for VLA:s. The risk analysis about this is up to you. But I might mention that the same is true for alloca. Although commonly available, it's not required by the standard.

Another problem is if the allocation fails. If you're using malloc, you have a chance to recover from this, but if you're only going to do something like this:

unsigned char *fileData = malloc(itemsToRead);
if(!fileData)
    exit(EXIT_FAILURE);

That is, just exit on failure and not trying to recover, then it does not really matter. At least not from a pure recovery point of view.

But also, although the C standard does not impose any requirement that VLA:s end up on the stack or heap, as far as I know it's pretty common to put them on the stack. This means that the risk of failing the allocation due to insufficient available memory is much, much higher. On Linux, the stack is usually 8MB and on Windows 1MB. In almost all cases, the available heap is much higher. The declaration char arr[n] is basically the same as char *arr = alloca(n) with the exception of how the sizeof operator works.

While I can understand that you might want to use the sizeof operator on a VLA sometimes, I find it very hard to find a real need for it. Afterall, the size can never change, and the size is known when you do the allocation. So instead of:

int arr[n];
...
for(int i=0; i<sizeof(arr), ...

Just do:

const size_t size = n;
int arr[size];
...
for(int i=0; i<size; ...

VLA:s are not a replacement for malloc. They are a replacement for alloca. If you don't want to change a malloc to an alloca, then you should not change to a VLA either.

Also, in many situations where a VLA would seem to bee a good idea, it is ALSO a good idea to check if the size is below a certain limit, like this:

int foo(size_t n)
{
    if(n > LIMIT) { /* Handle error */ }
    int arr[n];
    /* Code */
}

That would work, but compare it to this:

int foo(size_t n)
{
    int *arr = malloc(n*sizeof(*arr));
    if(!arr) { /* Handle error */ }
    /* Code */
    free(arr);
}

You did not really make things that much easier. It's still an error check, so the only thing you really got rid of was the free call. I might also add that it's a MUCH higher risk that a VLA allocation fails due to the size being too big. So if you KNOW that the size is small, the check is not necessary, but then again, if you KNOW that it is small, just use a regular array that will fit what you need.

However, I will not deny that there are some advantages of VLA:s. You can read about them here. But IMO, while they have those advantages they are not worth it. Whenever you find VLA:s useful, I would say that you should at least consider switching to another language.

Also, one advantage of VLA:s (and also alloca) is that they are typically faster than malloc. So if you have performance issues, you might want to switch to alloca instead of malloc. A malloc call involves asking the operating system (or something similar) for a piece of memory. The operating system then searches for that and returns a pointer if it finds it. An alloca call, on the other hand, is typically just implemented by changing the stack pointer in one single cpu instruction.

There are many things to consider, but I would avoid using VLA:s. If you ask me, the biggest risk with them is that since they are so easy to use, people become careless with them. For those few cases where I find them suitable, I would use alloca instead, because then I don't hide the dangers.

Short summary

• VLA:s are not required by C11 and later, so strictly speaking, you're relying on compiler extensions. However, the same is true for alloca. So if this is a very big concern, use fixed arrays if you don't want to use malloc.

• VLA:s are syntactic sugar (Not 100% correct, especially when dealing with multidimensional arrays) for alloca and not malloc. So don't use them instead of malloc. With the exception of how sizeof work on a VLA, they offer absolutely no benefit at all except for a somewhat simpler declaration.

• VLA:s are (usually) stored on the stack while allocations done by malloc are (usually) stored on the heap, so a big allocation has a much higher risk to fail.

• You cannot check if a VLA allocation failed or not, so it can be a good idea to check if the size is too big in advance. But then we have an error check just as we do with checking if malloc returned NULL.

• A VLA cannot be global nor static. The static part alone will likely not cause any problems whatsoever, but if you want a global array, then you're forced to use malloc or a fixed size array.

This function works fine.

No it does not. It has undefined behavior. As pointed out by Jonathan Leffler in comments, the array fileName is too short. It would need to be at least 12 bytes to include the \0-terminator. You can make this a bit safer by changing to:

snprintf(fileName, 
         sizeof(fileName), 
         "%s_%u%u%u%s", 
         "LOG", day, date, month, ".bin");

In this case, the problem with the too small array would manifest itself by creating a file with extension .bi instead of .bin which is a better bug than undefined behavior, which is the current case.

You also have no error checks in your code. I would rewrite it like this. And for those who thinks that goto is bad, well, it usually is, but error handling is both practical and universally accepted among experienced C coders. Another common use is breaking out of nested loops, but that's not applicable here.

int memory_get_log(unsigned char day, unsigned char date, unsigned char month){

    char fileName[12];
    unsigned long readItems, itemsToRead;
    int ret = 0;

    F_FILE *file;

    snprintf(fileName, 
             sizeof(fileName), 
             "%s_%u%u%u%s", "LOG", 
             day, date, month, ".bin");

    file = f_open(fileName , "r");
    if(!file) { 
        ret = 1; 
        goto END;
    }

    itemsToRead = f_filelength( fileName );

    unsigned char *fileData = malloc(itemsToRead);
    if(!fileData) { 
        ret=2;
        goto CLOSE_FILE;
    }
 
    readItems = f_read(fileData, 1, itemsToRead, file);
    // Maybe not necessary. I don't know. It's up to you.
    if(readItems != itemsToRead) {  
        ret=3;
        goto FREE;
    }

    // Assuming transmit_data have some kind of error check
    if(!transmit_data(fileData, itemsToRead)) {  
        ret=4;
    }

FREE:
    free(fileData);
CLOSE_FILE:
    f_close(file);
END:
    return ret;
}

If a function only returns 0, then it's pointless to return anything. Declare it as void instead. Now I used the return value to make it possible for the caller to detect errors and the type of error.