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.

Answer 3

Think of it this way: when you allocate memory for the struct, you get the pointer member variables for free. So in essence, when you do this:

mystruct *s = malloc(sizeof(mystruct)); //don't cast result of malloc.

Then you can treat s->str1 in the exact same way as you would any regular char* variable, say

char *str1 = NULL;

If you want it to point to something, then you have to allocate memory for the pointers. Consider this:

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

    char* someString = getMyString(); //gets some arbitrary string
    char* str1 = NULL;//just for demonstration
    int length = strlen(someString) + 1;

   //for struct members
    s->str1 = malloc(sizeof(char) * length);
    strcpy(s->str1, someString);

   //For regular pointers
    str1 = malloc(sizeof(char) * length);
    strcpy(str1, someString);

    return s;
}

Also note that if you just assign to a pointer by using the = operator instead of allocating memory, then it will only copy the address to the original value. This may or may not be what you want depending on the context. Generally, if you know the memory location will stay in scope and you don't need (or don't mind) to change the original string, then it is preferred to simply assign it. Otherwise, it is advisable to make a copy.

//Makes a copy of the string
s->str1 = malloc(sizeof(char) * length);
strcpy(s->str1, someString);

//copies the address of the original value only!
s->str1 = someString;

Answer 4

Use strncpy() instead of strcpy(). The latter is subject to buffer overruns.

For example in this code snippet given by another user, use the strncpy() in place of strcpy()

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

    char* someString = getMyString(); //gets some arbitrary string
    char* str1 = NULL;//just for demonstration
    int length = strlen(someString) + 1;

   //for struct members
   s->str1 = malloc(sizeof(char) * length);
   strcpy(s->str1, someString);

  //For regular pointers
  str1 = malloc(sizeof(char) * length);
  strcpy(str1, someString);   // replace with strncpy(str1, someString, bufsize);  where bufsize is the maximum number of characters in your string + 1 for the terminator '\0'.  

  return s;

}