Copying a string to a struct member

Question

I can't figure out how to copy a string from inputString to newNode->data.

My struct looks like this:

typedef struct node {
    char *data;
    struct node *left;
    struct node *right;
} node;

And the function in questions looks like this:

node* addToTree(char inputString[]) {
    node *newNode;

    if ((newNode = malloc(sizeof(node*))) == NULL) {
        printf("Error: could not allocate memory");
        exit(-1);
    }

    if ((newNode->data = malloc(strlen(inputString) + 1)) == NULL) {
        printf("Error: could not allocate memory");
        exit(-1);
    }

    /* This line of code doesn't seem to copy anything to newNode->data. 
       This is the way I believe should work, however I don't understand what the 
       problem with it is. I have tried strlcpy and strncpy as well. */
    strcpy(newNode->data, inputString);


    /* The line below here seems to work when I print the value
       within the function, but some of the values are garbage when
       I try to use them later on in the program. */
    newNode->data = inputString;


    newNode->left = NULL;
    newNode->right = NULL;
    printf("Input string: %s\n", inputString);
    printf("New node data: %s\n", newNode->data);

    return newNode;
}

`newnode = malloc(sizeof(node*))` ==> `newnode = malloc(sizeof (node))` or, better, `newnode = malloc(sizeof *newNode)` — pmg, Nov 05 '18 at 10:20
If you have, just use `strdup` to merge `malloc + strcpy` into one statement. — KamilCuk, Nov 05 '18 at 10:26

pmg · Accepted Answer · 2018-11-05T10:32:41.293

4

Your sizeof(node*) does not represent the size you need.

newnode = malloc(sizeof(node*))    // wrong
newnode = malloc(sizeof (node))    // correct
newnode = malloc(sizeof *newNode)  // better

Why is sizeof *newNode better?

Because it prevents accidental forgetting to update the code in two places if the type changes

struct node {
    char *data;
    struct node *next;
    struct node *prev;
};
struct nodeEx {
    char *data;
    size_t len;
    struct nodeEx *next;
    struct nodeEx *prev;
};

struct nodeEx *newnode = malloc(sizeof (struct node)); // wrong
struct nodeEx *newnode = malloc(sizeof *newnode);      // correct

edited Nov 05 '18 at 10:32

answered Nov 05 '18 at 10:23

pmg

106,608
13
126
198

I think you meant you write new*N*ode? – Kami Kaze Nov 05 '18 at 10:26
Why is using `sizeof *newNode` better than `sizeof (node)`? Wouldn't they always be the same size? – Jake Nov 05 '18 at 10:26
@Jake because the *newNode has always the the correct type. Imagine you change the type of a variable then you would have to change the sizeof for the allocation too. In this case it is not that relevant but is considered a good practice. – Kami Kaze Nov 05 '18 at 10:27

P.W · Answer 2 · 2018-11-05T11:02:25.377

1

The below line does not allocate the required amount of memory, it allocates memory equal to the size of a pointer to node.

if ((newNode = malloc(sizeof(node*))) == NULL)

So your strcpy fails because there is no memory to copy into.

Change the above to:

if ((newNode = malloc(sizeof(node))) == NULL)

What happens after you do the following is undefined behavior because the memory representing inputString can be overwritten, and that is why you get garbage values later on.

newNode->data = inputString;

You can see the top answer to this question for additional information.

edited Nov 05 '18 at 11:02

answered Nov 05 '18 at 10:23

P.W

26,289
6
39
76

@pmg: Yes. I was thinking of this scenario. https://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope/6445794 – P.W Nov 05 '18 at 10:48

score 0 · Answer 3 · answered Nov 05 '18 at 10:25

newNode->data = inputString;

is incorrect, it overrides the previously malloced memory.

if ((newNode->data = malloc(strlen(inputString) + 1)) == NULL) {
    printf("Error: could not allocate memory");
    exit(-1);
}
strcpy(newNode->data, inputString);

is enough to allocate memory and copy the string into it.

Copying a string to a struct member

3 Answers3

Linked