-1

Helo, I've got a homework assignment to write a simple shell in C, using fork(), malloc() and execv() and I have the following problem: I need to free the memory for a variable, which I'm returning in a function

char** parse_cmdline(const char* line){
    int size = strlen(line);
    char** array_of_strings = malloc((sizeof(char*)*(size)))
    char* pch = strtok(line," \n\t\r");
    int co = 0;
    int co2;
    while (pch != NULL)
    {
        array_of_strings[co]=(char*)malloc((sizeof(char)*strlen(pch))+1);
        strcpy(array_of_strings[co], pch);
        pch = strtok (NULL, " \n\t\r");
        ++co;
    }
    array_of_strings[co] = NULL;
    return array_of_strings; //that's the variable I need to free
}

And here is the whole program

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


char** parse_cmdline(const char* line){
    int size = strlen(line);
    char** array_of_strings = malloc((sizeof(char*)*(size)));
    char* pch = strtok(line," \n\t\r");
    int co = 0;
    int co2;
    while (pch != NULL)
    {
        array_of_strings[co]=(char*)malloc((sizeof(char)*strlen(pch))+1);
        strcpy(array_of_strings[co], pch);
        pch = strtok (NULL, " \n\t\r");
        ++co;
    }
    array_of_strings[co] = NULL;
    return array_of_strings;
}


int main(int argc, char *argv[]){
    char line[512];

    printf("Welcome to My shell!\n");
    while(1){
        printf(">$");
        gets(line);
        pid_t pid = fork();
        if (pid == -1){
            perror("");
        }else if(pid == 0){
            execvp(parse_cmdline(line)[0], parse_cmdline(line));
        }else{
            wait(pid);
        }
    }
    return 0;
}

Please, help me

PuhPuhalev
  • 9
  • 3
  • You need to ask a question. – Mad Physicist Jan 02 '14 at 18:12
  • 1
    "I need to free the memory for a variable, which I'm returning in a function" - then... why don't you... **just free it?** –  Jan 02 '14 at 18:12
  • 1
    You're allocating the data, is there something stopping you from freeing it with an opposing algorithm? – WhozCraig Jan 02 '14 at 18:13
  • Also, [do not cast the return value of `malloc()`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/605858#605858). –  Jan 02 '14 at 18:13
  • 1
    Interesting question: does memory allocated by the first program remain allocated after `execvp` executes, replacing the first program with a new executable? – chepner Jan 02 '14 at 18:13
  • @WhozCraig I can't free it after i've returned it and I can't free it before returning it, because then I can't return it – PuhPuhalev Jan 02 '14 at 18:19
  • Unrelated : Don't use `gets()` as it is both evil and deprecated. Unrelated: Also, assuming the number of possible parameters is top-limited to the number of chars in the input string is overkill. At *worst* you will require len/2+1 pointers, assuming each token is exactly one char wide. – WhozCraig Jan 02 '14 at 18:20
  • @PuhPuhalev You can't free it regardless if all you're doing is execvp'ing afterward, as your process is replaced. As I see it the bigger problem is you're invoking the same function *twice* on an unwritable string. as written I don't see how this even *compiles*. `strtok` requires the first param be non-const. Where did you get this code from? – WhozCraig Jan 02 '14 at 18:21
  • After fixing your code to pass `line` as a `char*` rather than a `const char*` you should be able to reverse the algorithm by enumerating your `char**` list of `char*`, and free each slot until you encounter a NULL, then free the `char**` itself. – WhozCraig Jan 02 '14 at 18:28

2 Answers2

1

If execvp() succedded, then there's no need to free your buffers, because execvp() replaces the process memory image with the one you are going to execute. That means that the memory map of the old process is discarded, thus freeing any allocated buffers.

But if execvp() fails, you will be still in your current process and you will be able to free your buffers:

else if(pid == 0)
{
   char **cmdline = parse_cmdline(line);
   if (execvp(cmdline[0], cmdline)<0)
   {
     for (int i=0;*cmdline[i];i++)
       free(cmdline[i]);
     free(cmdline);
   }
}
mcleod_ideafix
  • 11,128
  • 2
  • 24
  • 32
  • Wow, that blew my mind! Coming from windows background, I had no idea there functions that did such things. It seems very bizarre to me! This means that variables in scope that point to allocated memory suddenly become invalid, no? And if you've received the pointer as a parameter to a function that calls execvp(), you have no idea whether the pointer will be invalid after calling it? – Aaron Jan 03 '14 at 16:01
0

Well, if you really only want to free the buffer pointed at by the array_of_strings variable, just store the return value of parse_cmdline in a local variable and then call free() on it after you're done processing it.

However, you are also allocating strings for each index of array_of_strings... so it's not really good enough unless you free those as well. Problem is main() doesn't know how many there are in the char* array (char**). So, you could either terminate the array with a NULL pointer or return the size somehow. Then, you would loop through the array and free all the individual strings before freeing the array.

Also, there are lots of other problems with the code that you would want to fix in a non-academic setting, but maybe that's beyond the scope of your question :-)

Aaron
  • 594
  • 2
  • 12