Search code examples
crecursionmemory-leaksvalgrind

Memory leak in recursive function in C


My little C program is iterating through the working directory's contents and there is no problem with that, but I checked it with Valgrind and there are some mysterious leaks that I can't make disappear for days.

So here is the code:

struct record {
    char* fullPath;
    int wdlen; // working directory length
    int relplen; // relative path length
    int namlen; // name length
    bool folder; // is folder
    long modified;
};

void populateDB() {
    struct record** records = NULL;
    int entryCount = 0;
    bool error = false;

    listDirRecursive(workingDirectory, &entryCount, &records, &error);

    if (error) {
        goto cleanup;
    }

    for (int i = 0; i < entryCount; i++) {
        printfColor(C_MAGENTA, "%d. wdlen: %d,\t\trelPathLen: %d,\t\tnamelen: %d,\t\tfolder: %s,\t\tmod: %ld,\t\tpath: %s\n",
            i, records[i]->wdlen, records[i]->relplen, records[i]->namlen, records[i]->folder ? "true" : "false", records[i]->modified, records[i]->fullPath);
    }

    // initDatabase(&entryCount, &records);

cleanup:

    for (int i = 0; i < entryCount; i++) {
        free(records[i]->fullPath);
        free(records[i]);
    }
    free(records);
}

void listDirRecursive(const char* path, int* count, struct record*** records, bool* error) {
    if (*error) return;

    struct dirent** fileList = NULL;

    int fileCount = scandir(path, &fileList, NULL, alphasort);
    if (fileCount < 0) {
        printfColor(C_RED, "Couldn't open: %s\n", path);
        *error = true;
        return;
    }

    struct record** tmpRecords = malloc(fileCount * sizeof *tmpRecords);
    if (!tmpRecords) {
        printfColor(C_RED, "Couldn't allocate memory for records\n");
        *error = true;
        return;
    }

    struct record* act = NULL;
    struct stat filestat;
    int processedFiles = 0;

    for (int i = 0, wdlen = strlen(workingDirectory), plen = strlen(path); i < fileCount; i++) {
        if (strcmp(fileList[i]->d_name, ".") == 0 ||
            strcmp(fileList[i]->d_name, "..") == 0) continue;

        act = malloc(sizeof *act);
        if (!act) {
            printfColor(C_RED, "Couldn't allocate memory for actual record\n");
            *error = true;
            goto cleanup;
        }
        
        act->fullPath = malloc(plen + fileList[i]->d_namlen + 2);
        if (!act->fullPath) {
            printfColor(C_RED, "Couldn't allocate memory for actual record's full path\n");
            *error = true;
            goto cleanup;
        }

        strcat(strcat(strcpy(act->fullPath, path), "/"), fileList[i]->d_name);

        act->wdlen = wdlen + 1;
        act->relplen = strlen(act->fullPath) - ((wdlen + 1) + fileList[i]->d_namlen);
        act->namlen = fileList[i]->d_namlen;

        act->folder = fileList[i]->d_type == DT_DIR;

        stat(act->fullPath, &filestat);
        act->modified = filestat.st_mtimespec.tv_nsec;

        tmpRecords[processedFiles++] = act;
        
        if (fileList[i]->d_type == DT_DIR) {
            char* p = malloc(plen + fileList[i]->d_namlen + 2);
            strcat(strcat(strcpy(p, path), "/"), fileList[i]->d_name);

            listDirRecursive(p, count, records, error);

            free(p);
        }
    }

    if (processedFiles == 0) {
        goto cleanup;
    }

    struct record** shrinkedTmpRecords = realloc(tmpRecords, processedFiles * sizeof *shrinkedTmpRecords);
    if (!shrinkedTmpRecords) {
        if (verbose) printf("Shrinking tmpRecords failed. Some memory leak will happen\n");
    }
    else {
        tmpRecords = shrinkedTmpRecords;
    }

    int newCount = *count + processedFiles;

    if (*count == 0) {
        *records = tmpRecords;
    }
    else {
        struct record** newRecords = realloc(*records, newCount * sizeof *newRecords);
        if (!newRecords) {
            printfColor(C_RED, "Couldn't allocate memory for further records\n");
            *error = true;
            goto cleanup;
        }

        *records = newRecords;
        for (int i = 0, j = *count; i < processedFiles; i++, j++) {
            (*records)[j] = tmpRecords[i];
        }
    }

    *count = newCount;

cleanup:

    for (int i = 0; i < fileCount; i++) {
        free(fileList[i]);
    }
    free(fileList);
}

After running, Valgrind's output is this:

==40861== HEAP SUMMARY:
==40861==     in use at exit: 18,084 bytes in 170 blocks
==40861==   total heap usage: 423 allocs, 253 frees, 180,357 bytes allocated
==40861== 
==40861== 8 bytes in 1 blocks are definitely lost in loss record 1 of 43
==40861==    at 0x100111FD6: realloc (in /usr/local/Cellar/valgrind/HEAD-e0af3eb/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==40861==    by 0x10000162B: listDirRecursive (filesystem.c:137)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x100001092: populateDB (filesystem.c:44)
==40861==    by 0x100000E15: init (mtl.c:111)
==40861==    by 0x100000CD2: main (mtl.c:61)
==40861== 
==40861== 16 bytes in 2 blocks are definitely lost in loss record 5 of 43
==40861==    at 0x100111FD6: realloc (in /usr/local/Cellar/valgrind/HEAD-e0af3eb/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==40861==    by 0x10000162B: listDirRecursive (filesystem.c:137)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x100001092: populateDB (filesystem.c:44)
==40861==    by 0x100000E15: init (mtl.c:111)
==40861==    by 0x100000CD2: main (mtl.c:61)
==40861== 
==40861== 72 bytes in 5 blocks are definitely lost in loss record 22 of 43
==40861==    at 0x100111FD6: realloc (in /usr/local/Cellar/valgrind/HEAD-e0af3eb/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==40861==    by 0x10000162B: listDirRecursive (filesystem.c:137)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x100001092: populateDB (filesystem.c:44)
==40861==    by 0x100000E15: init (mtl.c:111)
==40861==    by 0x100000CD2: main (mtl.c:61)
==40861== 
==40861== 80 bytes in 1 blocks are definitely lost in loss record 23 of 43
==40861==    at 0x100111FD6: realloc (in /usr/local/Cellar/valgrind/HEAD-e0af3eb/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==40861==    by 0x10000162B: listDirRecursive (filesystem.c:137)
==40861==    by 0x100001092: populateDB (filesystem.c:44)
==40861==    by 0x100000E15: init (mtl.c:111)
==40861==    by 0x100000CD2: main (mtl.c:61)
==40861== 
==40861== 112 bytes in 4 blocks are definitely lost in loss record 26 of 43
==40861==    at 0x100111FD6: realloc (in /usr/local/Cellar/valgrind/HEAD-e0af3eb/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==40861==    by 0x10000162B: listDirRecursive (filesystem.c:137)
==40861==    by 0x1000015DA: listDirRecursive (filesystem.c:127)
==40861==    by 0x100001092: populateDB (filesystem.c:44)
==40861==    by 0x100000E15: init (mtl.c:111)
==40861==    by 0x100000CD2: main (mtl.c:61)
==40861== 
==40861== LEAK SUMMARY:
==40861==    definitely lost: 288 bytes in 13 blocks
==40861==    indirectly lost: 0 bytes in 0 blocks
==40861==      possibly lost: 0 bytes in 0 blocks
==40861==    still reachable: 0 bytes in 0 blocks
==40861==         suppressed: 17,796 bytes in 157 blocks

The 288 bytes divided by 8 (the size of the pointer) is 36, which is the total records (files and folders) at the end. The 13 blocks count equals the number of folders.

I tried everything that came to my mind, but nothing worked, I'm learning C, so it's possible that I don't know something essential (I read a lot in the topic and tried to avoid the big no-nos).

If somebody doesn't understand what is that printfColor, here it is:

// header file:
enum Color {
    C_RED,
    C_BOLD_RED,
    C_GREEN,
    C_BOLD_GREEN,
    C_YELLOW,
    C_BOLD_YELLOW,
    C_BLUE,
    C_BOLD_BLUE,
    C_MAGENTA,
    C_BOLD_MAGENTA,
    C_CYAN,
    C_BOLD_CYAN,
    C_RESET
};

static const char* colors[] = {
    "\033[0;31m", // RED
    "\033[1;31m", // BOLD RED
    "\033[0;32m", // GREEN
    "\033[1;32m", // BOLD GREEN
    "\033[0;33m", // YELLOW
    "\033[01;33m", // BOLD YELLOW
    "\033[0;34m", // BLUE
    "\033[1;34m", // BOLD BLUE
    "\033[0;35m", // MAGENTA
    "\033[1;35m", // BOLD MAGENTA
    "\033[0;36m", // CYAN
    "\033[1;36m", // BOLD CYAN
    "\033[0m" // RESET
};

// c file:
void printfColor(enum Color color, const char* format, ...) {
    va_list args;

    printf("%s", colors[color]);
    va_start(args, format);
    vprintf(format, args);
    va_end(args);
    printf("%s", colors[C_RESET]);
}

Chears


Solution

  • Shawn is correct in the comments. tmpRecords is allocated and used as a set of pointers:

    struct record** shrinkedTmpRecords = realloc(tmpRecords, processedFiles * sizeof *shrinkedTmpRecords);
    if (!shrinkedTmpRecords) {
        if (verbose) printf("Shrinking tmpRecords failed. Some memory leak will happen\n");
    }
    else {
        tmpRecords = shrinkedTmpRecords;
    }
    

    Then, if count is 0, you are just assigning records to tmpRecords:

    if (*count == 0) {
        *records = tmpRecords;
    }
    

    Later this will be freed up top. Here you free the elements of records as well as the set of pointers that were allocated to hold them.

    for (int i = 0; i < entryCount; i++) {
        free(records[i]->fullPath);
        free(records[i]);
    }
    free(records);
    

    If the count is not zero in during an iteration, we allocate more space for records. Then, you loop through tmpRecords and assign the elements to records. These elements will be freed when records is freed, but the set of pointers allocated for tmpRecords itself are never freed. I've added a line in the code below that will fix this leak.

    if (*count == 0) {
        *records = tmpRecords;
    }
    else {
        struct record** newRecords = realloc(*records, newCount * sizeof *newRecords);
        if (!newRecords) {
            printfColor(C_RED, "Couldn't allocate memory for further records\n");
            *error = true;
            goto cleanup;
        }
    
        *records = newRecords;
        for (int i = 0, j = *count; i < processedFiles; i++, j++) {
            (*records)[j] = tmpRecords[i];
        }
        free(tmpRecords);  /*** THIS LINE ***/
    }