Trouble with dangling pointers and character arrays in C

Question

main(){
   char *cmd1[20] = {NULL};
   int x = parse_command(cmd1);
   printf("%s\ ",cmd1[0]);
}

parse_command(char *inTempString){
   char tempString[256];
   (call to function that assigns a string to tempString)
   cmd1[0] = tempString;
}

There is a problem when printing out the cmd1[0] within main. I am pretty sure that it is a dangling pointer error. I don't really know how to go about fixing it.

Answer 1

There are two issues with your program.

First, when you say:

char *cmd1[20] = {NULL};

cmd1 is an array of 20 pointers to char. This means that cmd1[i] for i in [0,20) is a pointer to char.

There is a rule in C that says that passing an array to a function only passes the pointer to the first element of the array to the function. I.e., if you had code like:

int ai[20];
f(ai);

then the type of ai in the function call f(ai); is int * and the pointer passed to f() is equal to &ai[0], the first element of ai.

So, when you do:

parse_command(cmd1);

you immediately know that the "thing" passed to parse_command() is &cmd1[0], i.e., a pointer to the first element of cmd1. Since cmd1[0] is of type char *, you are passing a char ** to parse_command. Therefore, your declaration:

parse_command(char *inTempString);

is wrong, you should do:

parse_command(char **inTempString);

to match your call. This assumes that parse_command() will parse more than one value in cmd1. If that is the case, you should also pass the number of elements in cmd1 to parse_commnd()—since it can't know how many elements cmd1 has.

Your second problem is that you can't return the address of a local variable from a function in C. As above, in addition to a function call, returning an array in C, or assigning something to an array in C also makes the name of an array "decay" to a pointer to its first element.

So given your function:

/* changed inTempString to cmd1 because that's what you probably meant */
int parse_command(char *cmd1)
{
    char tempString[256];
    /* call to function that assigns a string to tempString */
    cmd1[0] = tempString;
    /* you need to return an int from here */
}

the tempString in the assignment to cmd1[0] is actually &tempString[0], i.e., a pointer to the first element of tempString. But since tempString is destroyed as soon as the function returns, the pointer becomes invalid. You can't use the value later.

In fact, in C, the name of an array decays to a pointer to its first element in all cases, except:

• when used as an operand to sizeof operator, and
• when used as an operand to the address-of (&) operator

To be more precise, in object contexts, the name of an array doesn't decay to a pointer, and in value contexts, it decays to a pointer. See this for more details.

Now, how should you fix your second issue? It depends—you can either allocate memory dynamically in parse_command(), and assign that memory to cmd1[0], or you can make tempString static in the function. Since static variables in a function are not destroyed when a function returns, you can continue using a pointer to it. Dynamic allocation is more work—you need to worry about allocation failure and you need to remember to free the pointer when done. static array is easier, but you have to be careful because another call to parse_command will overwrite the array, making it less-generic.

Assuming you want to go the "dynamic memory" route, here is a scheme that you could use:

#include <stdio.h> /* printf */
#include <stdlib.h> /* malloc and free */

int main(void) /* main returns int */
{
    char *cmd1[20] = {NULL};
    /* number of commands.  "sizeof cmd1" is the number of bytes
       used by the cmd1 array, and "sizeof cmd1[0]" is the number
       of bytes used by one element of the array.  The division
       gives you the number of elements.  This is 20 of course
       but doing it this way makes sure that changing "20" to any
       number works. */
    size_t ncmds = sizeof cmd1 / sizeof cmd1[0];
    /* pass the number of commands to "parse_command", since
       it can't know otherwise */
    int x = parse_command(cmd1, ncmds);
    int i;
    for (i=0; i < x; ++i) {
        printf("%s ", cmd1[i]);
        free(cmd1[i]);
    }
    return 0; /* return a value from main */
}

int parse_command(char **cmd1, size_t ncmds)
{
    char *tempString; /* we will malloc this */
    int i; /* the number of mallocs done successfully */
    tempString = malloc(...);
    if (tempString == NULL) {
    /* failure, handle gracefully */
    } else {
        ++i; /* make sure i doesn't exceed or equal ncmds */
    }
    cmd1[0] = tempString;
    /* do the above as many times as you need */
    return i; /* the number successfully assigned to */
}

Answer 2

You're declaring cmd1 in main as a char** -- that is, a pointer to pointer to char. However, you then pass it to parse_command, which you've defined as taking a char*; a pointer to char.

This only compiles because of automatic conversion of pointer-to-anything to pointer-to-char. That's a historical artifact of old versions of C that used 'char*' in place of 'void*'; in your case, it just means that the compiler is ignoring the type error that you made rather than reporting it to you.

Answer 3

Yeah, you can't do that.

char tempString[256];

declares a variable on the stack in the function parse_command, that variable goes out of scope and pointers to it cease to be valid when parse_command returns.

You need to allocate the command string on the heap, so that it will still be valid when parse_command returns. This is one way.

parse_command(char *inTempString){
   char tempString[256];
   (call to function that assigns a string to tempString)

   int cb = strlen(tempString)+1;
   cmd1[0] = (char *)malloc(cb);
   strcpy(cmd1[0], tempString);
}

You should also call free(cmd[0]) before main exits.

In addition to that, this code doesn't compile. You can't reference cmd1[0] from inside the parse_command function. You should be getting a type mismatch when you try and pass cmd1 into parse_command, If you mean to return a char* from parse_command then it should be declared to take a char** as an argument, more like this.

parse_command(char **pcmd){
   char tempString[256];
   (call to function that assigns a string to tempString)

   int cb = strlen(tempString)+1;
   pcmd[0] = (char *)malloc(cb);
   strcpy(pcmd[0], tempString);
}

Answer 4

Something like this will work:

parse_command(char **inTempString){
        static char tempString[256];
        strcpy(tempString,"some string you want to copy");
        inTempString[0] = tempString;
}

• In your code the tempString would not exist once the function returns. You need to keep it alive even after the function returns. You can allocate the space dynamically and de-allocate later or you can declare it as static.
• Also you need to change the type argument inTempString from char* to char**.

Answer 5

You are trying to access cmd1 variable that is inside main function from parse_command.

I would say that at least the cmd1[0] will look like an integer because it is not declared withing that method.

The cmd1 is declared as array of char* but the parameter to method is char* which might be a pointer to char array but not pointer to array of char*.

The best way to copy char array into another char array is to use either memcpy,strcpy or similar methods that accept pointers to src, dest and size to be copied.