Strcpy resulting in segmentation fault

Question

I'm not really sure why strcpy is resulting in a segmentation fault and was wondering if someone could explain to me why. I originally had temp->data = name but that resulted in the Node value changing every time I changed the name array and was looking for a solution

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



node *create(char name[]){
        node *temp;
        temp = (node *) malloc(strlen(name) + 1);
        strcpy(temp->data, name);
        temp->left = temp->right = NULL;
        return temp;
}

You aren't allocating enough space for the structure. You need to allocate `sizeof(*temp)` (or `sizeof(node)`) and also the space for a string. Since you're not using a flexible array member, you probably need two allocations; your one is certainly not enough. And also `temp->data` is not initialized; it isn't pointing to any known memory. — Jonathan Leffler, Feb 18 '18 at 04:33

Jonathan Leffler · Accepted Answer · 2018-02-18T04:53:11.960

Given the structure shown, you are allocating insufficient memory and copying to an uninitialized pointer. Both are dangerous.

You need something more like:

node *create(char name[]){
    node *temp = malloc(sizeof(*temp));
    if (temp == NULL)
        return NULL;
    temp->data = malloc(strlen(name) + 1);
    if (temp->data == NULL)
    {
        free(temp);
        return NULL;
    }
    strcpy(temp->data, name);
    temp->left = temp->right = NULL;
    // temp->generation = 0; // removed from revised question
    return temp;
}

Consider whether you can use strdup() to allocate a copy (duplicate) of the string. You'd still need to check that was successful. Note that freeing a node involves two calls to free(). Also, the calling code needs to check whether the node was successfully allocated. However, this code imposes no error handling strategy on its caller — the calling code can do what it likes as long as it doesn't try to dereference a null pointer returned by the code.

Alternatively, you could use a C99 'flexible array member' like this:

typedef struct BST {
    struct BST *left;
    struct BST *right;
    char data[];
} node;


node *create(char name[]){
    node *temp = malloc(sizeof(*temp) + strlen(name) + 1);
    if (temp == NULL)
        return NULL;
    strcpy(temp->data, name);
    temp->left = temp->right = NULL;
    // temp->generation = 0; // removed from revised question
    return temp;
}

Now you can free the structure with a single free() call. However, you can't create an array of these structures (though you could have an array of pointers to such structures). In the context of a tree, that's unlikely to be a problem.

Chongju Mai · Answer 2 · 2018-02-18T04:44:35.620

3

You should malloc your node first with temp = (node *) malloc(sizeof(node)); then malloc your new string with temp->data = (char *) malloc(strlen(name) + 1); then you can use strcpy(temp->data, name); Also, you need to set your generation to whatever value you want.

edited Feb 18 '18 at 04:44

answered Feb 18 '18 at 04:35

Chongju Mai

127
1
8

1

Curious that this answer cast here `(node *) malloc(sizeof(node));`, but not here `malloc(strlen(name) + 1);`. Recommend no casts. – chux - Reinstate Monica Feb 18 '18 at 04:38
Well, turned out that we don't have to cast it. So you can put the cast or not. It should not affect the result – Chongju Mai Feb 18 '18 at 04:41
https://stackoverflow.com/questions/1565496/specifically-whats-dangerous-about-casting-the-result-of-malloc - please read – Ed Heal Feb 18 '18 at 04:42
3

Agreed that the casts can be present or absent, so a mixture doesn't affect the result. However, programmers should be consistent, at least within a single piece of code (function, file, program, system — choose the size of your 'piece of code'). Inconsistency inside a single function smacks more of carelessness than careful thought. – Jonathan Leffler Feb 18 '18 at 04:43
Make those changes. But yes you detected it right..get UV. – user2736738 Feb 18 '18 at 05:17

Strcpy resulting in segmentation fault

2 Answers2