Weird Side Effect with Malloc()

Question

So i'm writing a C program for a simple shell. Not too difficult but I've run into one really odd problem that I can't explain. I'm attempting to create a two dimensional array within a struct to represent a command and its arguments. For the command "ls -l" for example i'd like to have the first element "ls" and the second "-l". Seems to work fine other than that malloc changes the "ls" to "ms". The same thing for other commands, the first character is incremented; not before the malloc and immediately afterward.

This is the hunk of code in question....

    printf ("PRE MALLOC: %c\n", ret_val->args[0][0]);
    printf ("[0] %p\n", ret_val->args[0]);
    ret_val->args[1] = (char*) malloc ((3) * sizeof (char));
    printf ("[0] %p\n", ret_val->args[0]);
    printf ("[1] %p\n", ret_val->args[1]);
    printf ("POST MALLOC: %c\n", ret_val->args[0][0]);

All I'm attempting to accomplish is to allocate a 3 ( -l + the null ) character array to hold the "-l" in args[1]. These aren't really going to be hardcoded either but I figured it makes the point better.

The output produced it this...

PRE MALLOC: l

[0] 80613b0

[0] 80613b0

[1] 80613b8

POST MALLOC: m

So the two addresses don't overlap or anything strange but the first character of the first array is incremented? I'm sorry if I'm overlooking something stupid. But I can't think of any reason why this would happen.

Any ideas?

Thanks,

Here is much more code, for some context.

int lcv;
int numargs;
int numchars;
int offset;
int bool;

TCommand* ret_val;
char** tmp;

ret_val = (TCommand*) malloc ( sizeof (TCommand) );
ret_val->cmd = NULL;
ret_val->args = NULL;

/* CMD */
lcv = 0;
numargs = 0;
numchars = 0;
offset = 0;

/* Remove initial whitespace */
while (input[offset] == ' ')
{
    ++offset;
}

lcv = offset;

/* Loop through command string */
while ( input[lcv] != ' ' && input[lcv] != 0 && input[lcv] != '&')
{
    ++numchars;
    ++lcv;
}

ret_val->cmd = (char*) malloc ( (numchars+1) * sizeof(char));

/* Copy to command string */
memcpy (ret_val->cmd, &(input[offset]), (numchars * sizeof (char)));
ret_val->cmd[numchars] = 0;
offset += numchars;

/* Copy command string into first argument */
ret_val->args = (char**) malloc ( sizeof (char*));
memcpy (ret_val->args[numargs++],ret_val->cmd, (numchars+1) * sizeof(char));

bool = 1;
while ( bool )
{

    /* Remove initial whitespace */
    while (input[offset] == ' ')
    {
        ++offset;
    }

    lcv = offset;

    if ( input[lcv] == 0 )
    {
        bool = 0;
    }
    else
    {
        ++numargs;
        tmp = (char**) realloc (ret_val->args, numargs * sizeof (char*));
        ret_val->args = tmp;
        numchars = 0;

        while ( input[lcv] != ' ' && input[lcv] != 0 &&
            input[lcv] != '&')
        {
            ++numchars;
            ++lcv;
        }

                printf ("PRE MALLOC: %c\n", ret_val->args[0][0]);
                printf ("[0] %p\n", ret_val->args[0]);
                    ret_val->args[1] = (char*) malloc ((2) * sizeof (char));
                printf ("[0] %p\n", ret_val->args[0]);
                printf ("[1] %p\n", ret_val->args[1]);
                printf ("POST MALLOC: %c\n", ret_val->args[0][0]);
                fflush(stdout);
        memcpy (ret_val->args[numargs-1],&(input[offset]),numchars * sizeof (char));
        ret_val->args[numargs-1][numchars] = 0;
        offset += numchars;
    }
}

Answer 1

This sounds like a classic example of heap corruption.

What you are seeing is most probably the result of the malloc() call overwriting the string that args[0] points to with its return value. This can happen if the args array does not actually have enough space for the second argument. By writing into args[1] you are overwriting the contents of the string that is pointed to by argv[0] and that I suspect you allocate with an strdup() call or similar immediately after allocating the args array.

Since the heap grows upwards, and assuming no intervening free() calls, by overstepping the bounds of a dynamically allocated array there is an increased chance of messing with the area allocated immediately afterwards.

I'll hazard a guess and recommend that you check the allocated size for the args array by printing out the arguments for the realloc() call and checking its return value.

EDIT:

Apparently not in this case. It seems that the argv[0] pointer could actually be uninitialised, without causing a segmentation fault. Interesting...

Answer 2

This code:

/* Copy command string into first argument */
ret_val->args = (char**) malloc ( 2 * sizeof (char*));
memcpy (ret_val->args[numargs++],ret_val->cmd, (numchars+1) * sizeof(char));

Copies through the uninitialised pointer ret_val->args[0]. Try:

/* Copy command string into first argument */
ret_val->args = malloc(2 * sizeof ret_val->args[0]);
ret_val->args[numargs] = malloc(numchars + 1);
memcpy(ret_val->args[numargs++], ret_val->cmd, numchars + 1);

(Note that sizeof(char) is defined to be 1 by the language).

Answer 3

I guess we need to see how args[0] is allocated, and how it is given a value.

I can make a few suggestions, though they won't fix the allocation problem:

1. It is not necessary to cast the result of malloc(), and while one has limited ability to change a type in an already-written C program, there is no reason to deliberately lower the level of abstraction by open-coding duplicate type information.

   Old texts and old code examples often cast malloc() because C did not always have a void * type, and even after it arrived people wrote in the old style by habit and for code portability to old compilers. But you are unlikely these days to ever need to compile for the PDP-11.

2. sizeof(char) is, by C99, 1, so it's OK to just say malloc(3), and it reads slightly easier because it's shorter. And if you know it's "-l", you may as well just use "-l".

Answer 4

The problem isn't in the fragment you show; it is somewhere in the code you don't show.

So, you need to build your example from the ground up - showing the structure declarations, the memory management and copying you do for the structures.