Pointers to char* inside a struct in C

Question

I have a probably stupid question... but I have defined a struct with some char* inside. Now when I try to change the values of that chars. It doesn't give problems when it's compiled, but when I execute it, the program stops.

This is a test function that I made for checking where is the problem:

struct myret {
    int age;
    char *name;
    char *affiliation_number;
};

void obtain_name_affiliation_number(struct myret *r)
{
    int age;
    char *name;
    char *affiliation_number;

    FILE *user_data;
    user_data = fopen("user_data.txt", "r");

    fscanf(user_data, "%d", &age);
    fscanf(user_data, "%s", &name);
    fscanf(user_data, "%s", &affiliation_number);

    fclose(user_data);

    r->age = age;
    r->name = name;
    r->affiliation_number = affiliation_number;

    return 0;
}

int main(void)
{
    struct myret r;
    int rc = obtain_name_affiliation_number(&r);
    if (rc == 0) {
        printf("%d %s %s\n", r.age, r.name, r.affiliation_number);
    }

    getchar();
    return 0;
}

Thanks for everything =)

Answer 1

First at all you must use malloc(); and free(); when you're using char* a="always use malloc";

compiler uses it as const char* a

1. You must assign memory to your char* a; when a will be used as a variable string
2. Use free(); after it to avoid memory leaks

Answer 2

First, you need to malloc memory for your data, otherwise there will be nowhere to store it:

char *affiliation_number = (char*) malloc(STRING_LENGTH * sizeof (char));

If malloc fails, it will return NULL, you have to check for this error condition:

if (!affiliation_number) { 
     // report error
}

Then, affiliation_number is already a pointer, you don't need & in:

fscanf(user_data, "%s", affiliation_number);

Same for name.

Answer 3

You've allocated space for only the addresses of buffers for name and affiliation_number, but never allocated the buffers themselves for those addresses to point to.

Hence, when you use them in fscanf(), problems occur. The compiler warns you (gcc, anyway) with these notes that the pointers are the wrong type - they should be the address of the first byte in the target buffer you want fscanf to overwrite:

foo.c:19:5: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘char **’ [-Wformat]
foo.c:20:5: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘char **’ [-Wformat]

Here are some things to do instead - if your system doesn't support fscanf's "%ms" spec, try this:

• In obtain_name_affiliation, make a char buffer[1024] to read data into.
• Use buffer or &buffer[0] (for certain purists) as the targets in the string fscanfs
• Use %1023s or the like to prevent the data read from overrunning the length of the buffer - overruns can drive your program insane.
• If the fscanf returns success (for one field expected, fscanf should return the value 1, or the input may be bad), use strdup to clone the data into name or affiliation_number. This will malloc() a new memory piece sized to fit the string you read and copy the data into it.

Whether you use those steps or the "%ms" approach, the buffers need to be free()ed later to avoid memory leaks!

This is a bit simplistic (the limit of 1024 in particular), but should get you started down the right path.

Example:

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

/* tested with user_data.txt containing "12 Barney 42" or "12 Barney" */

struct myret {
    int age;
    char *name;
    char *affiliation_number;
};

int obtain_name_affiliation_number(struct myret *r)
{
    int success = 0;
    int age;
    char *name = 0;
    char *affiliation_number = 0;

    FILE *user_data = fopen("user_data.txt", "r");
    if(user_data)
    {
#if 0  /* use if you have "%ms" */
        if((1 == fscanf(user_data, "%d",  &age)) &&
           (1 == fscanf(user_data, "%ms", &name)) &&
           (1 == fscanf(user_data, "%ms", &affiliation_number)))
           {
                success = 1;
           } else {
               /* a small annoyance: if only the first "%ms" succeeded,
                * we need to free it:
                */
               if(name)
                   free(name);
           }
#else
        char buffer[1024];
        /* This if-structure can be used with the "%ms" as well, and
         * would make the "annoyance" look a lot cleaner
         */
        if(1 == fscanf(user_data, "%d", &age))
        {
            if(1 == fscanf(user_data, "%1023s", buffer))
            {
                name = strdup(buffer);
                if(1 == fscanf(user_data, "%1023s", buffer))
                {
                    affiliation_number = strdup(buffer);
                    success = 1;
                }
            }
        }
#endif
        fclose(user_data);
    } else perror("error opening data file");

    if(success)
    {
        r->age = age;
        r->name = name;
        r->affiliation_number = affiliation_number;
    }
    return success;
}

int main(void)
{
    struct myret r;
    int rc = obtain_name_affiliation_number(&r);
    if(rc) {
        printf("%d %s %s\n", r.age, r.name, r.affiliation_number);
        free(r.name);
        free(r.affiliation_number);
    }
    else
        fputs("an error occurred reading data\n", stderr);
    getchar();
    return 0;
}

There are other approaches. The name and affiliation_number could be declared in the struct as char name[512]; for example, but that number is almost impossible to choose in advance with any hope of correctness. Instead, this is common:

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

struct myret *myret_alloc(int age, char *name, char *affiliation_number)
{
    struct myret *r = 0;
    if(r = (struct myret*)calloc(1, sizeof(struct myret))) {
        r->age = age;
        r->name = name;  /* or use r->name = strdup(name); */
        r->affiliation_number = affiliation_number;
        /* note that using strdup would mean the calling function should
         *  do its own cleanup of its own name and affiliation_number vars
         */
    }
    return r;
}

void myret_free(struct myret *r)
{
    /* this can be called on partially-allocated myret objects */
    if(r->affiliation_number)
        free(r->affiliation_number);
    if(r->name)
        free(r->name);
    free(r);
}

Then the other two functions become (assuming fscanf can "%ms"):

struct myret *obtain_name_affiliation_number(void)
{
    struct myret *r = (struct myret*)0;
    FILE *user_data = fopen("user_data.txt", "r");
    if(user_data)
    {
        int age;
        char *name = 0;  /* the 0 allows us to see if it was used later */
        char *affiliation_number = 0;

        if((1 == fscanf(user_data, "%d",  &age)) &&
           (1 == fscanf(user_data, "%ms", &name)) &&
           (1 == fscanf(user_data, "%ms", &affiliation_number)))
        {
            /* The name and affiliation_number were malloc()ed by "%ms"
             * so there's nothing to clean up in this function, and
             * we can let myret_free() just free those memory areas.
             * This also means myret_alloc doesn't need strdup().
             */
            r = myret_alloc(age, name, affiliation_number);
        } else {
            if(name) /* clean up name if it got allocated */
                free(name);
        }
        fclose(user_data);
    } else perror("error opening data file");
    return r;
}

int main(void)
{
    struct myret *r = obtain_name_affiliation_number();
    if(r) {
        printf("%d %s %s\n", r->age, r->name, r->affiliation_number);
        myret_free(r);
    }
    else
        fputs("an error occurred reading data\n", stderr);
    getchar();
    return 0;
}

You could also use fscanf for all three at once:

if(3 == fscanf(user_data, "%d %ms %ms", &age, &name, &affiliation_number))
      ...

Good luck!

Answer 4

Just declaring a char * does not allocate any memory for it. You either need to specify a static size (e.g. char name[100]) or need to allocate memory dynamically (e.g. by using malloc). Remember to free the memory if you use malloc when you are done.

Another problem is the use of & in fscanf for char *. The variable list in scanf and fscanf after format string is a list of pointers where to place the input values. When we use type like int age we need to pass &age, as that & before age makes it a pointer to age, i.e. a int *. But char *name is already a pointer, so you don't need & here. By using &name you are creating a pointer to pointer or char ** which you don't want here.

Answer 5

You don't need to use & to pass the address of a char *, remove thoses &:

fscanf(user_data, "%s", &name);
                        ^
fscanf(user_data, "%s", &affiliation_number);
                        ^

Also, allocate memory for name and affiliation_number before using them by malloc for example:

char *name = malloc(100 * sizeof(char));