Free variables at C program

Question

Im trying to write and implement the command 'tree -pugs' in linux on C languange. It running well but I have leak issues when I run the program with valgrind flag. I have tried to free the variable but I got segmentation fault.

heres my code:

int count_dir = 0;
int count_file = 0;
int count_total = 0;
char *user_name = "";
char *group_name = "";
long file_size = 0;
char last_type;
char *pre_name;
mode_t pre_mode;
int tree_walk(const char *name, const struct stat *status, int type, struct FTW *ftw)
{
    
    if (type == FTW_D || type == FTW_F)
    {
        curr_level = ftw->level;

        if (pre_level != 0 && (count_file + count_dir != 0))
        {
            
            for (size_t i = 0; i < 9; i++)
            {
                printf("%c", Permissions[i]);
            }
            
            printf(" %s\t%s %15ld]  %s\n", user_name, group_name, file_size, pre_name);

            if ((count_dir + count_file) == count_total - 1)
            {
                for (size_t i = 0; i < 9; i++)
                {
                    printf("%c", Permissions[i]);
                }
                printf(" %s\t%s %15ld]  %s\n\n", user_name, group_name, file_size, pre_name);
            }
        }

        if (type == FTW_D && strcmp(".", name) != 0)
            count_dir++;
    }
    return 0;
}

int main(int argc, char const *argv[])
{
    int flag = 0;

    if (argc == 1)
    {
        nftw(".", tree_walk_counter, 10, flag);
        nftw(".", tree_walk, 10, flag);
    }
    else if (argc == 2)
    {
        nftw(argv[1], tree_walk_counter, 10, flag);
        nftw(argv[1], tree_walk, 10, flag);
    }
    else
    {
        fprintf(stderr, "write ./stree \"directory name\" or just ./stree\n");
        exit(1);
    }
    char * dirs;
    char * files; 
    if (count_dir == 1) dirs = "directory";
    else dirs = "directories";
    if (count_file == 1) files = "file";
    else files = "files";
    
    printf("%d %s, %d %s\n", count_dir, dirs, count_file, files);
    
    return 0;
}

when i run with valgring:

==7132== 
==7132== HEAP SUMMARY:
==7132==     in use at exit: 1,158 bytes in 182 blocks
==7132==   total heap usage: 369 allocs, 187 frees, 595,272 bytes allocated
==7132== 
==7132== LEAK SUMMARY:
==7132==    definitely lost: 1,137 bytes in 179 blocks
==7132==    indirectly lost: 0 bytes in 0 blocks
==7132==      possibly lost: 0 bytes in 0 blocks
==7132==    still reachable: 21 bytes in 3 blocks
==7132==         suppressed: 0 bytes in 0 blocks
==7132== Rerun with --leak-check=full to see details of leaked memory
==7132== 
==7132== For lists of detected and suppressed errors, rerun with: -s
==7132== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Any advice? I tried to put free on the variables but it did segmentation fault

Answer 1

This is a showcase for why casting the results of malloc is a poor decision in C. You are hiding a number of errors by explicitly casting pointer values (void *) to integer values (intptr_t), which then get implicitly converted to their left-hand side types (mode_t, char, long).

Explicit casts should be reserved for when you know exactly what you are doing. They should not be used to hammer solutions into place.

The main issue here is that you have largely misunderstood when it is appropriate to allocate dynamic memory, and how to do so.

For example, this global definition

mode_t pre_mode;

already allocates enough memory for a mode_t object. Additionally, its lifetime is the lifetime of your entire program. This rules out the two main reasons to use dynamic memory allocation in the first place.

These lines

    pre_mode = (intptr_t)(malloc(sizeof(lStatus.st_mode)));
    pre_mode = lStatus.st_mode;

are totally conflicted. The first line allocates sizeof (mode_t) bytes of memory, takes the resulting void * and explicitly casts it to an inptr_t, which in turn is implicitly cast to a mode_t value.

We can see -Wconversion highlights an issue with this

stree.c:187:28: warning: conversion from ‘long int’ to ‘mode_t’ {aka ‘unsigned int’} may change value [-Wconversion]
  187 |                 pre_mode = (intptr_t)(malloc(sizeof(lStatus.st_mode)));

as a potential change in the value might result in the memory address being the incorrect value to pass to free, even if cast back to an appropriate pointer type.

This value is discarded in the second line when you overwrite it. The memory is absolutely leaked at this point in time.

Likewise,

*Permissions = (intptr_t)(malloc(9));

allocates nine bytes, casts to an integer, and then places this value in *Permissions (a.k.a, Permissions[0]), as a char value.

Similar issues for last_type and file_size.

None of these require dynamic memory allocation, they already have all the memory they need, and are not of the correct type to hold memory addresses anyway.

---

Conversely, user_name and group_name do have a use for dynamic memory if they are to hold a variable length string, copied from getpwuid and getgrgid return values. It is of course important to have your own memory for these, as the structures returned by these functions likely occupy static memory, and will be overwritten by subsequent calls.

The problem is your attempted use of sizeof to determine the length of a string. The sizeof operator does not do this, as it is for determining the size (in bytes) of an object or type.

To figure out the length of a string, you use strlen. To have enough memory to hold this string and its null-terminating byte ('\0') you add one to this value.

struct passwd *pwd = getpwuid(lStatus.st_uid);         
user_name = malloc(strlen(pwd->pw_name) + 1);                 
strcpy(user_name, pwd->pw_name);

Oddly enough, you do this moments later with pre_name:

 pre_name = malloc(strlen(name + ftw->base) + 1);

Alternatively, you could use statically allocated buffers of reasonable sizes, and ensure any copied strings will not exceed these limits.

---

Finally, every single call to malloc will, at some point in time, require a mirrored call to free.

If you call malloc 7 times in a loop, you must also eventually call free 7 times, passing in each of the values returned from malloc.

Additionally, malloc can fail, returning NULL. You should consider what to do in this event.

---

Also,

if (file_name[i] == '/' & file_name[i + 1] == '.')

basic compiler warnings would let you know that this expression probably wants logical and (&&) not bitwise and (&). That said, this condition and its surrounding loop could be simplified to

if (name[ftw->base] == '.')