copying strings into a buffer

Question

Hello,

I have some code that stack dumps when using sprintf to copy a a pointer to strings. I am trying to copy the contents of animals into a new pointer array called output. However, I get a stack dump.

What should be in the output is the following: new animal rabbit new animal horse new animal donkey

An I going about this the right way?

Many thank,s

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

void p_init(const char **animals, char **output);

int main(int argc, char **argv)
{
    char *animals[] = {"rabbit", "horse", "donkey", '\0'}; 
    char **prt_animals = animals;
    char *output[sizeof(*animals)];

        /* print the contents here */
    while(*prt_animals)
    {
     printf("Animal: %s\n", *prt_animals++);
    }

        /* copy and update in the output buffer */
    p_init(*&animals, *&output);

    getchar();

    return 0;


void p_init(const char **animals, char **output)
{
    while(*animals)
    {
     sprintf(*output, "new animal %s", *animals); 
     *output++;
    }
}

Answer 1

First of all, what's the point of

p_init(*&animals, *&output);

as opposed to

p_init(animals, *&output);

?

Second, it is illegal to convert a char** to a const char** for reasons explained here.

Finally, your main problem is that the test

while (*animals)

which you are expecting to fail when you've reached the empty string at the end of the animals array, is wrong. That statement is actually checking if the pointer which points to the string is NULL, not checking if the string is EMPTY. An empty string (a string containing the single character '\0'), is not the same as a null pointer.

In other words, once you've reached the last element of the animals array, *animals evaluates to a non-NULL pointer which happens to point to a string that is empty. Therefore the test passes, and your loop continues forever (well, it continues until you run far enough past the end of the animals array that it causes a segfault).

You can fix this either by replacing '\0' with NULL when you create the animals array, or you can change the while check to check strlen(*animals) == 0 (or any other way of checking for an empty string instead of a null pointer.

Edit: Note that others pointed out equally serious problems that I missed.

Answer 2

char *output[sizeof(*animals)];

Creates an array of size 4 of pointers to char. It does not, however, allocate memory to those pointers. These array members contain garbage (i.e. point to memory you don't own). Trying to write to that memory invokes UB. In your case, UB manifests itself by a stack dump. Some other problem with your function p_init is noted below:

void p_init(const char **animals, char **output)
{
    /* runs an infinite loop -- since *animals is never incremented */
    /* need to pass size of *animals so that you can terminate your loop */
    while(*animals)
    {
        /* allocate some memory */
        sprintf(*output, "new animal %s", *animals); 
        *output++;
    }
}

A fixed up code will be something like this:

void p_init(const char ** animals, const size_t nanimals, char **output)
{
    const char **w = animals;
    size_t len = 0;
    while (w < animals + nanimals)
    {
        len = strlen(*w);
        *output = malloc(len + sizeof "new animal " + 1);
        sprintf(*output, "new animal %s", *w); 
        output++;          
        w++;
    }
}

int main(int argc, char **argv)
{
    char *a[] = { "rat", "dog", "lion" };
    char *o[ sizeof a/ sizeof *a ];
    p_init((const char**)a, sizeof a / sizeof *a, o);
    for (size_t i = 0; i < sizeof a / sizeof *a; ++i) printf("%s\n", o[ i ]);
    for (size_t i = 0; i < sizeof a / sizeof *a; ++i) free(o[ i ]);
    return 0;
}

Feel free to throw in the required headers.

Answer 3

You haven't allocated any space in your output array to put the copy into. You'll need to use malloc to allocate some space before using sprintf to copy into that buffer.

void p_init(const char **animals, char **output)
{
    while(*animals)
    {
        size_t stringSize = 42; /* Use strlen etc to calculate the size you need, and don't for get space for the NULL! */
        *output = (char *)malloc(stringSize);
        sprintf(*output, "new animal %s", *animals); 
        output++;
        animals++;
    }
}

Don't forget to call free() on that allocated memory when you are done with it.

Answer 4

You are not creating a very big buffer - sizeof(*animals) is sizeof(char*) which is 4 bytes on a 32bit system; you're not creating anywhere to put the output strings; and you are not using a safe mechanism such as snprintf to write to the buffer, so you crash rather than failing safe.

Fixes for these follow, keeping to your zero-terminated array use:

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

void p_init ( const char **animals, char **output );

int main(int argc, char **argv) {
    // no reason to use char 0 rather than int 0 to mark end here
    const char* animals[] = {"rabbit", "horse", "donkey", "pig", 0};

    printf("sizeof(*animals) = %zd\n", sizeof(*animals));
    printf("number of elements in animals = %zd\n", sizeof(animals) / sizeof(*animals));

    char *output[sizeof(animals)/sizeof(*animals)];

    // print animals
    for ( const char**p = animals; *p; ++p)
        printf ( "Animal: %s\n", *p );

    // format animals to output
    p_init ( animals, output);

    // print output
    for ( char**p = output; *p; ++p)
        printf ( "Animal: %s\n", *p );

    // free output
    for ( char**p = output; *p; ++p )
        free(*p);

    return 0;
}

void p_init ( const char **animals, char **output ) {
    while ( *animals ) {
        size_t  len = strlen ( *animals );
        char*   buf = malloc ( len + 13 );

        snprintf ( buf, len + 13, "new animal %s", *animals );

        *output = buf;

        ++animals;
        ++output;
    }

    *output = 0;
}

Answer 5

The array animals is an array of pointers. It is not an array of buffers of some size. Therefor, if you do

sizeof(*animals)

You will get the sizeof of the first element of that array. Equivalent to

sizeof(char*)

Because your array stores pointers. So, in the line that reads

char *output[sizeof(*animals)];

You allocate 4 or 8 pointers in one array (depends on how wide a pointer on your platform is. Usually it's either 4 or 8). But that's of course not senseful! What you wanted to do is create an array of pointers of the same size as animals. You will have to first get the total size of the animals array, and then divide by the size of one element

char *output[sizeof(animals)/sizeof(*animals)];

Now, that is what you want. But the pointers will yet have indeterminate values... Next you pass the array using *&animals (same for the other). Why that? You can pass animals directly. Taking its address and then dereference is the same as doing nothing in the first place.

Then in the function you call, you copy the strings pointed to by elements in animal to some indeterminate destination (remember the elements of the output array - the pointers - have yet indeterminate values. We have not assigned them yet!). You first have to allocate the right amount of memory and make the elements point to that.

while(*animals) {
        // now after this line, the pointer points to something sensible
        *output = malloc(sizeof("new animal ") + strlen(*animals));
        sprintf(*output, "new animal %s", *animals); 
        output++; // no need to dereference the result
        animals++; // don't forget to increment animals too!
}

Addition, about the sizeof above

There's one important thing you have to be sure about. It's the way we calculate the size. Whatever you do, make sure you always have enough room for your string! A C string consists of characters and a terminating null character, which marks the end of the string. So, *output should point to a buffer that is at least as large so that it contains space for "new animal " and *animals. The first contains 11 characters. The second depends on what we actually copy over - its length is what strlen returns. So, in total we need

12 + strlen(*animals)

space for all characters including the terminating null. Now it's not good style to hardcode that number into your code. The prefix could change and you could forget to update the number or miscount about one or two characters. That is why we use sizeof, which we provide with the string literal we want to have prepended. Recall that a sizeof expression evaluates to the size of its operand. You use it in main to get the total size of your array before. Now you use it for the string literal. All string literals are arrays of characters. string literals consist of the characters you type in addition to the null character. So, the following condition holds, because strlen counts the length of a C string, and does not include the terminating null character to its length

// "abc" would have the type char[4] (array of 4 characters)
sizeof "..." == strlen("...") + 1

We don't have to divide by the size of one element, because the sizeof char is one anyway, so it won't make a difference. Why do we use sizeof instead of strlen? Because it already accounts for the terminating null character, and it evaluates at compile time. The compiler can literally substitute the size that the sizeof expression returns.