free() a buffer consumed by system()

Question

A question i could not find anything about on the internet. I have this little piece of C-code running on a Linux Wheezy distro (Raspberry Pi, but thats not relevant):

void function(const char * command)
{

    // Define commands for in between parameters
    char commandPre[] = "echo ";

    // Get the lengths of the strings
    int len= strlen(command) + strlen(commandPre);


    // Allocate the command
    char * fullCommand = (char *) malloc(len * sizeof(char));

    // Build the command
    strcat(fullCommand, commandPre);
    strcat(fullCommand, command);


    // Execute command
    system(fullCommand);

    // Free resources
    free(fullCommand);
}

Now, I'm running this piece of code from a daemon program. But when it reaches free(fullCommand) a second time (when function gets called a second time in my program), the program crashes and exists. When i remove the free(fullCommand), it works as expected.

My question is: Is system() already freeing "fullCommand" for me? If so, why does it work the second time and not the first time? Am I missing something here?

P.S. Actually command is build up of several strings strcat'ed together, but above is the code in its most basic form

score 3 · Accepted Answer · edited May 23 '17 at 12:28

3

You have a buffer overrun, since you're not allocating space for the string terminator.

Also, don't cast the return value of malloc(), and check the return value before assuming the allocation worked.

Also, as you point out in your own answer, using strcat() on a newly allocated buffer is broken since the buffer won't be an empty string. Sorry for not picking that up earlier.

edited May 23 '17 at 12:28

Community

1
1

answered Jun 04 '13 at 11:53

unwind

391,730
64
469
606

Ah RIGHT, stupid stupid stupid me *hits head*. Thanks! I've been programming C# for a while now and i completely forgot abot the terminator. Also, i always cast it like this because it is fast but checking the return value is better practice, thanks for the tip! – Maarten Jun 04 '13 at 11:58
@Maarten "It is fast"? It can't be faster (to type, I assume you mean) than *not* typing the cast, can it? There is no performance difference, and the cast is in no way "worse" than checking the return value, those are completely different things. The cast will happily convert a NULL to a character pointer, and then things will break. – unwind Jun 04 '13 at 13:18

Maarten · Answer 2 · 2013-06-05T06:52:19.753

I found my error:

    // Allocate the command
    char * fullCommand = (char *) malloc(len * sizeof(char));

    // Build the command
    strcat(fullCommand, commandPre);

There is no guarantee that fullCommand is empty after a malloc. strcat places the second argument's first character in the place of the first arguments terminator. However, the terminator might or might not appear on the first location of the allocated array since the data in the memory after a malloc is random. Fixed it by doing:

// Allocate the command
char * fullCommand = calloc(len, sizeof(char));

Alternatively, I could have done:

// Allocate the command
char * fullCommand = malloc(len * sizeof(char));
fullCommand[0] = '\0';

Or als Alk pointed out in the comments, start with a strcpy:

// Allocate the command
char * fullCommand = malloc(len * sizeof(char));

// Build the command
strcpy(fullCommand, commandPre);

Or you could have just used `strcpy()` instead of the first call to `strcat()`. — alk, Jun 04 '13 at 16:25
Your last example causes a memory leak, as you lose the address returned by `malloc()` by overwriting it with the address of `""`. You propably meant `fullCommand[0] = '\0'`. — alk, Jun 04 '13 at 16:30

free() a buffer consumed by system()

2 Answers2