Initialise struct pointers in a function in C

Question

I'm fairly new to C and I'm a little confused as to the correct way to initialise struct variables which are pointers, within a function. Is this style sufficient, or do I need to allocate memory before I assign s->str? Thank you kindly for your replies, apologies if the question is unclear as I am very new to this language.

typedef struct Mystruct{
    const char* str1;
    const char* str2;
}mystruct;

mystruct* mystruct_new(const char* str1, const char* str2){
    mystruct *s = (mystruct*)(malloc(sizeof(mystruct)));
    s->str1 = str1;
    s->str2 = str2;
    return s;
}

Answer 1

your function is legal and doesn't do anything bad. Nevertheless, you should document it to mention that the strings are not copied, only the pointers are.

So if the passed data has a shorter life than the structure itself, you may meet undefined behaviour. Example:

mystruct*func()
{
   char a[]="foo";
   char b[]="bar";

   return mystruct_new(a,b);
}
mystruct*func2()
{
   char *a="foo";
   char *b="bar";

   return mystruct_new(a,b);
}

int main()
{
    mystruct *s = func();
    printf(s->a); // wrong, memory could be trashed
    mystruct *s2 = func2();
    printf(s2->a); // correct
    mystruct *s3 = mystruct_new("foo","bar");
    printf(s3->a); // also correct, string literals have global scope
}

the above code is undefined behaviour for the first print because s->a points to some memory that is no longer allocated (local to func). The second print is OK because s2->a points to a string literal which has infinite life span.

So maybe your function is more useful like this:

mystruct* mystruct_new(const char* str1, const char* str2){
    mystruct *s = malloc(sizeof(mystruct));
    s->str1 = strdup(str1);
    s->str2 = strdup(str2);
    return s;
}

now the memory is allocated for the strings. Don't forget to free it when discarding the structure, better done in another utility function.

Answer 2

If the strings being passed in to str and str2 will always be string constants than yes you can get away with doing it this way. My guess however is that this is not the case. So you would be better off making a copy of each string with strdup and assigning those to the struct members:

mystruct* mystruct_new(const char* str1, const char* str2){
    mystruct *s = malloc(sizeof(mystruct));
    s->str1 = strdup(str1);
    s->str2 = strdup(str2);
    return s;
}

Just make sure to free each of those fields before freeing the struct.