Freeing a char**

[+Azazel+]

Hello guys! I have a problem with freeing allocated memory.

What I have is a structure called "vars" in which I have a char **env.

This env will contain all the environment variables of the shell. So I copy everything into my char **env.

What I did is:

vars->env = xmalloc(sizeof(*vars->env) * 200);
vars->env[199] = NULL;
  while (environ[i] != NULL)
    {
      vars->env[i] = xmalloc(sizeof(**vars->env) * 500);
      vars->env[i] = &environ[i][0];
      i++;
    }

Now when I want to free, I have to free vars-env AND every element of it right?

But now, when I do free(vars->env[1]) it gives me an error:

==2908== Invalid free() / delete / delete[]
==2908== at 0x3C034687: free (in /usr/local/lib/valgrind/vgpreload_memcheck.so)
==2908== by 0x8049E60: my_exit (in ./mysh)
==2908== by 0x8048FF5: is_built_in_command (in ./mysh)
==2908== by 0x80488C6: main (in ./mysh)
==2908== Address 0x4FFFEE12 is on thread 1's stack

Freeing just vars->env works, but I want to free everything (so that there are 0 leaks).

Thank you!

[dwks]

It looks okay in theory . . . for example, this works:

#include <stdio.h>
#include <stdlib.h>

int main() {
    char **data;
    const size_t X = 100, Y = 200;
    size_t x;

    data = malloc(sizeof(*data) * X);
    for(x = 0; x < X; x ++) {
        data[x] = malloc(sizeof(**data) * Y);
    }

    for(x = 0; x < X; x ++) {
        free(data[x]);
    }

    free(data);

    return 0;
}

I think one of two things are happening.

• You're calling free(data) before free(data[x]).
• More likely, you're only allocating, say, data[0] to data[50], but freeing data[0] to data[500].

[+Azazel+]

Sorry for bothering, I found the mistake. It was here:

vars->env[i] = &environ[i][0];

As far as I am guessing I was assigning the memory address of environ to vars->env so when I was freeing vars->env, I actually was trying to free environ.

Now Im doing the old fashioned way:

 while (x < my_strlen(environ[i]))
        {
          vars->env[i][x] = environ[i][x];
          x++;
        }
i++;

[cas]

In your original code:

vars->env[i] = xmalloc(sizeof(**vars->env) * 500);
vars->env[i] = &environ[i][0];

This is a memory leak (given the sane assumption that xmalloc() is a malloc()-like function). What you're doing is assigning the result of malloc() to a variable, then immediately overwriting that value with your next assignment. Incidentally that's probably why you got the crash: you wind up doing, essentially, free(&environ[i][0]) because that's now what vars->env[i] points to.

Your current method (the "old-fashioned" way) simply makes copies of pointers; that's not necessarily wrong. If you know that the data to which the pointers point won't be freed out from under you, or that it won't be changed, etc, then you're fine.

However, if you wanted to make copies of strings into newly allocated storage, you'd do it something like this (assume all allocations succeed):

char *new_string = malloc(strlen(old_string) + 1);
strcpy(new_string, old_string);
/* or for those who think that this needs "optimization" */
char *new_string;
size_t l;
l = strlen(old_string);
new_string = malloc(l + 1);
memcpy(new_string, old_string, l + 1);