Is repetitively creating buffers on the stack in a loop in C bad practice?

Question

The title of this post is very similar to what I searched in relation to this. Every result I came across was about buffer overflows, and that isn't what I'm after.

I have my function iterating through each filename in a dirent structure I've previously populated. Each filename varies in size, from being very small, to being quite large.

Previously what my function would do, is create the buffer, with a size of 2048 bytes. Then enter the loop. During each iteration of the loop, the buffer is filled with a path to the target directory, plus the current filename within the directory concatenated onto the end of it. With the new path within the buffer, I perform some fairly small file operations. This happens until the final filename in the structure has been reached.

The problem however is that not every full path is going to be 2048 bytes. Some may not even a third of this size.

Revisiting this function, I moved the creation of the buffer to within the loop, and each iteration of the loop creates the buffer, of size n, where n is the length of the target directory + the length of the current filename within the directory.

I am wondering whether or not this may be considered bad practice or anything otherwise. Am I better off creating the buffer beforehand & always having a set size for it, even if 2/3 of the buffer is unused sometimes? Or is it a better idea to only create the buffer for exactly the size of what it is I need?

I hope I've given ample information... Thanks in advance!

Here is the function in question.

int verifyFiles(DIR *dp, const char *pathroot){
    struct dirent *dir;
    struct stat pathstat;
    //char path[2048];
    int status = 0;

    while((dir = readdir(dp)) != NULL){
        if(!strncmp(dir->d_name, ".", 1))
            continue;

        size_t len = strlen(pathroot) + strlen(dir->d_name) + 2;
        char path[len];
        snprintf(path, sizeof(path), "%s/%s", pathroot, dir->d_name);
        
        // verify shebang is present on the first line of path's contents.
        if(!shebangPresent(path)){
            status = -1;
            break;
        }

        // verify path belongs to the user.
        stat(path, &pathstat);
        if(pathstat.st_uid != getuid()){
            status = -1;
            break;
        }
    }

    return status;
}

It would help me at least understand your issue if you post what code you have now. — 500 - Internal Server Error, Jul 10 '20 at 16:11
*"The problem however is"* - Is it really a problem? If so, how? Don't fix what aint broken. — klutt, Jul 10 '20 at 16:29
Updated with sample code. Thanks. @klutt Surely it's just better practice if I tend to not waste space unnecessarily? It's not a matter of something being broke or working. It's a matter of wanting to be more efficient wherever I can. And if this is somewhere I can make my code more efficient then I should know... — topal, Jul 10 '20 at 16:30

klutt · Accepted Answer · 2020-07-10T16:39:19.943

There is absolutely nothing wrong with having a fixed buffer like that. Don't worry about such small details. The function will allocate 2kB of memory, do it's job and then release it. If that's a problem, then you have bigger problems than this piece of code.

I would only worry about such things in the case of recursive functions. Like if you had something like this:

int foo(int n) 
{
    char buf[2048];
    int r = foo(n-1);
    // Do something with buf and return
}

The above code would quickly eat up the stack for large n. But in your case I really would not worry until you have some proof or at least reasonable suspicion that it's actually causing a problem.

If it was a much larger buffer, say in the order of 100kB, then I would definitely use dynamic allocation. The stack typically has 1MB on Windows and 8MB on Linux. So this is not a matter of "not wasting memory", it's about not blowing up the stack.

Thanks for this reply, appreciated muchly. – topal Jul 10 '20 at 16:37 — topal, Jul 10 '20 at 16:37

chux - Reinstate Monica · Answer 2 · 2020-07-10T17:07:21.183

Is repetitively creating buffers on the stack in a loop in C bad practice?
I am wondering whether or not this may be considered bad practice or anything otherwise.

No, char path[len]; is not a problem.

Yet the approach used here to determine the buffer size is weak.

Below code repeatedly calculates the string length of pathroot. Maybe, a good compiler may analyse and see a repeated call is not needed. Easy enough to ensure the calculation is done once.

size_t pathlen = strlen(pathroot); // add

while((dir = readdir(dp)) != NULL){
    if(!strncmp(dir->d_name, ".", 1))
        continue;

    // size_t len = strlen(pathroot) + strlen(dir->d_name) + 2;
    size_t len = pathlen + strlen(dir->d_name) + 2;

    char path[len];

Am I better off creating the buffer beforehand & always having a set size for it, even if 2/3 of the buffer is unused sometimes?

In this case, the path size likely has an environmental upper limit: perhaps available as MAXPATH or MAXPATHLEN.

I'd consider the below to avoid repeated copying the path - it might be quite long.

char path[MAXPATH + 1];  // or malloc()
int len = snprintf(path, sizeof path, "%s/", pathroot);
if (len < 0 || len >= sizeof path) Handle_OutOfRoom();

while((dir = readdir(dp)) != NULL){
  int len2 = snprintf(path + len, sizeof path - len, "%s", dir->d_name);
  if (len2 < 0 || len2 >= sizeof path - len) Handle_OutOfRoom();
  ...

Is repetitively creating buffers on the stack in a loop in C bad practice?

2 Answers2