CODEWITHSUNDEEP
cstrcpy
antelemon
antelemon

Reputation: 55

Strcpy resulting in segmentation fault

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;
}

Upvotes: 0

Views: 166

Answers (2)

Jonathan Leffler
Jonathan Leffler

Reputation: 753475

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.

Upvotes: 4

Chongju Mai
Chongju Mai

Reputation: 155

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.

Upvotes: 3

Related Questions