Search code examples
cmemory-leaksmallocvalgrind

Memory leak when passing allocated array through recursing function


This is the function list_directory where I lose allocated pointer and I cant free it later. This should be ls-a-like implementation and when it finds a directory it should save its name and after listing the directory it should call list_directory on the directories found in the current directory recursively. But for some reason it loses some elements on the way.

int list_directory(int argc, char *argv[], struct check_info *chinfo, int dirs)
{
    struct group *grp;
    struct passwd *pwd;
    struct stat file_info;
    struct tm *mtime;
    struct dirent *dir_info;

    int subdirs = 0;
    int printsubdir = 0;
    int skip_newline = 1;

    char timebuffer[26];
    char **recursqueue;

    if (chinfo->param_R)
    {
        if ((recursqueue = malloc(argc * sizeof(char*))) < 0)
            perror("malloc");
    }

    if (dirs > -1)
    {
        for (int i = 0; i < chinfo->files; i++)
        {
            list_file(chinfo->argv_files[i], chinfo);
            free(chinfo->argv_files[i]);
        }
        free(chinfo->argv_files);
    }

    for (int i = 1; i < argc; i++)
    {
        DIR *dp = opendir(argv[i]);

        if (dp)
        {
            if (chinfo->param_R)
            {
                if ((recursqueue[subdirs] = malloc((256) * sizeof(char))) < 0)
                    perror("malloc");
                strcpy(recursqueue[subdirs++], "./ls");
            }

            if (dirs+chinfo->files > 1 || chinfo->param_R)
            {
                if (!skip_newline || dirs == -1 || chinfo->files)
                    printf("\n%s:\n", argv[i]);
                else
                    printf("%s:\n", argv[i]);
            }

            skip_newline = 0;

            if (chinfo->param_l)
            {
                dir_indent_info(argv[i], chinfo);
                printf("total %lu\n", chinfo->blocks_total/2);

            }

            while ((dir_info = readdir(dp)) != NULL)
            {
                if (!strcmp(".", dir_info->d_name) ||
                    !strcmp("..", dir_info->d_name))
                {
                    continue;
                }
                else if (!strncmp(".", dir_info->d_name, 1)
                         && !chinfo->param_A)
                {
                    continue;
                }
                else
                {
                    char path[strlen(argv[i]) + strlen(dir_info->d_name) + 1];
                    sprintf(path, "%s/%s", argv[i], dir_info->d_name);

                    if (lstat(path, &file_info) == -1)
                        continue;

                    switch (file_info.st_mode & S_IFMT)
                    {
                        case S_IFBLK:  printf("b"); break;
                        case S_IFCHR:  printf("c"); break;
                        case S_IFDIR:  printf("d");
                        if (chinfo->param_R)
                        {
                            printsubdir = 1;
                            recursqueue[subdirs] = malloc((256) * sizeof(char));
                            strcpy(recursqueue[subdirs++], path);
                        }
                        break;
                        case S_IFIFO:  printf("p"); break;
                        case S_IFLNK:  printf("l"); break;
                        case S_IFREG:  printf("-"); break;
                        case S_IFSOCK: printf("s"); break;
                        default:       printf("?"); break;
                    }

                    if (chinfo->param_l)
                    {
                        printf( (file_info.st_mode & S_IRUSR) ? "r" : "-");
                        printf( (file_info.st_mode & S_IWUSR) ? "w" : "-");
                        printf( (file_info.st_mode & S_IXUSR) ? "x" : "-");
                        printf( (file_info.st_mode & S_IRGRP) ? "r" : "-");
                        printf( (file_info.st_mode & S_IWGRP) ? "w" : "-");
                        printf( (file_info.st_mode & S_IXGRP) ? "x" : "-");
                        printf( (file_info.st_mode & S_IROTH) ? "r" : "-");
                        printf( (file_info.st_mode & S_IWOTH) ? "w" : "-");
                        printf( (file_info.st_mode & S_IXOTH) ? "x" : "-");
                        printf(" %*lu", numlen(chinfo->link_len), file_info.st_nlink);

                        pwd = getpwuid(file_info.st_uid);
                        printf(" %-*s", chinfo->usr_len, pwd->pw_name);

                        grp = getgrgid(file_info.st_gid);
                        printf(" %-*s", chinfo->grp_len, grp->gr_name);

                        printf(" %*lu", numlen(chinfo->size_len), file_info.st_size);
                        mtime = localtime(&file_info.st_mtime);
                        strftime(timebuffer, 26, "%b %e %R", mtime);
                        printf(" %s", timebuffer);

                    }

                    printf(" %s\n", dir_info->d_name);
                }
            }
            if (printsubdir)
            {
                if((list_directory(subdirs, recursqueue, chinfo, -1)) < 0)
                    printsubdir = -1;

                for (int i = 0; i < subdirs; i++)
                {
                    free(recursqueue[i]);
                }
                if (printsubdir < 0)
                    return -1;
            }
            closedir (dp);
            subdirs = 0;
        }
    }
    if (chinfo->param_R)
        free(recursqueue);
    return 0;
}

I pass argv to dir_indent_info so here it is:

int dir_indent_info(char* dirpath, struct check_info *chinfo)
{
    struct dirent *dir_info;
    struct stat file_info;
    struct group *grp;
    struct passwd *pwd;

    reset_info(chinfo);

    DIR *dp = opendir(dirpath);
    if (dp)
    {
        while ((dir_info = readdir(dp)) != NULL)
        {
            if (!strcmp(".", dir_info->d_name) ||
                !strcmp("..", dir_info->d_name))
            {
                continue;
            }

            char path[strlen(dir_info->d_name) + strlen(dirpath) + 1];

            sprintf(path, "%s/%s", dirpath, dir_info->d_name);

            if (lstat(path, &file_info) == -1)
            {
                perror("lstat()");
                continue;
            }

            pwd = getpwuid(file_info.st_uid);
            if (strlen(pwd->pw_name) > chinfo->usr_len)
                chinfo->usr_len = strlen(pwd->pw_name);

            grp = getgrgid(file_info.st_gid);
            if (strlen(grp->gr_name) > chinfo->grp_len)
                chinfo->grp_len = strlen(grp->gr_name);

            chinfo->blocks_total += file_info.st_blocks;

            if (file_info.st_size > chinfo->size_len)
                chinfo->size_len = file_info.st_size;

            if (file_info.st_nlink > chinfo->link_len)
                chinfo->link_len = file_info.st_nlink;
        }
        closedir(dp);
    }
    else
    {
        perror("error");
        return -1;
    }
    return 0;
}

Valgrind output:

==6361== WARNING: new redirection conflicts with existing -- ignoring it
--6361--     old: 0x0401cdc0 (strlen              ) R-> (0000.0) 0x3809e181 ???
--6361--     new: 0x0401cdc0 (strlen              ) R-> (2007.0) 0x04c31020 strlen


==6361== 
==6361== HEAP SUMMARY:
==6361==     in use at exit: 768 bytes in 3 blocks
==6361==   total heap usage: 231 allocs, 228 frees, 596,108 bytes allocated
==6361== 
==6361== Searching for pointers to 3 not-freed blocks
==6361== Checked 65,288 bytes
==6361== 
==6361== 256 bytes in 1 blocks are definitely lost in loss record 1 of 2
==6361==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6361==    by 0x4016FD: list_directory (ls.c:293)
==6361==    by 0x401DFB: list_directory (ls.c:385)
==6361==    by 0x401F55: main (ls.c:415)
==6361== 
==6361== 512 bytes in 2 blocks are definitely lost in loss record 2 of 2
==6361==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6361==    by 0x4016FD: list_directory (ls.c:293)
==6361==    by 0x401DFB: list_directory (ls.c:385)
==6361==    by 0x401DFB: list_directory (ls.c:385)
==6361==    by 0x401F55: main (ls.c:415)
==6361== 
==6361== LEAK SUMMARY:
==6361==    definitely lost: 768 bytes in 3 blocks
==6361==    indirectly lost: 0 bytes in 0 blocks
==6361==      possibly lost: 0 bytes in 0 blocks
==6361==    still reachable: 0 bytes in 0 blocks
==6361==         suppressed: 0 bytes in 0 blocks
==6361== 
==6361== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 1 from 1)

Allocating pointers for the char** array:

if (chinfo->param_R)
{
     if ((recursqueue = malloc(argc * sizeof(char*))) < 0)
         perror("malloc");
}

Allocating the first element and assigning ./ls to it to simulate an argument input:

if (chinfo->param_R)
{
     if ((recursqueue[subdirs] = malloc((256) * sizeof(char))) < 0)
         perror("malloc");
     strcpy(recursqueue[subdirs++], "./ls");
}

If the current file is a directory we add it in the recursqueue

case S_IFDIR:  printf("d");
     if (chinfo->param_R)
     {
          printsubdir = 1;
          recursqueue[subdirs] = malloc((256) * sizeof(char));
          strcpy(recursqueue[subdirs++], path);
     }

Calling the same function with the collected sub-directories' paths:

if (printsubdir)
{
    if((list_directory(subdirs, recursqueue, chinfo, -1)) < 0)
        printsubdir = -1;

    for (int i = 0; i < subdirs; i++)
    {
        free(recursqueue[i]);
    }
    if (printsubdir < 0)
         return -1;
}

Solution

  • Your code has numerous issues, such as huge functions, uninitialized variables, vogue names of variables, reuse of variables for unrelated purposes, unnecessary broad scope of the variables, obscure ownership of allocated memory, assignments in conditional expressions, less-than comparison of pointer with 0 etc.

    As for memory leaks, they can easily happen with such code, for example:

    • you allocate the first recursqueue item for ./ls string, but if printsubdir is not set to 1 then it will never be freed;
    • you allocate recursqueue array, but if list_directory returns values less than 0 then it will never be freed (and closedir is not called either);