Segmentation Fault error - binary search tree in C

Question

I'm trying to build a binary search tree. Inserting an integer using insert function (only using 1 to 100 for testing) and appending the result to a file using inorder traversal. However, i'm getting a segmentation fault error. Using Visual Studio code on Macbook Pro 2020. Also tested on Codeblocks on Windows - filename.exe stops working and crashes.

#include <stdio.h>
#include <stdlib.h>
    
typedef struct node *BST;
    
struct node {
    int data;
    BST left;
    BST right;
};

BST insert(BST root, int number) {
    BST temp = NULL;
    if (root == NULL) {
        temp = *(BST *)malloc(sizeof(BST));
        temp->left = NULL;
        temp->right = NULL;
        temp->data = number;
        root = temp;
        }
    else if (root->data > number) {
        insert(root->left, number);
    }
    else {
        insert(root->right, number);
    }
    return root;
}

//returning null if number not found and pointer to root if found
BST find(BST root, int number) {
    if (root == NULL) {
        return NULL;
    }
    
    if (root->data > number) {
        find(root->left, number);
    }
    else if (root->data < number) {
        find(root->right, number);
    }
    else (root->data = number);
        return root;
}
    
//printing number to file
void inOrder (BST root, FILE *fp) {
    if (root == NULL) return;
    inOrder(root->left, fp);
    fprintf(fp, "%d", root->data);
    inOrder(root->right, fp);
}

int main() {
    BST root = NULL;
    int n = 100;
    int treeArray[n];
    for (int r = 0; r < n; r++) {
        treeArray[r] = r;
    }
    root = insert(root, treeArray[0]);
    for (int x = 1; x < n; x++) {
        insert(root, treeArray[x]);
    }
    
    FILE *treefile = fopen("print_tree.txt", "w");
    inOrder(root, treefile);
    fclose(treefile);
    return 0;
}
    
Error: /bin/sh: line 1: 44278 Segmentation fault: 11 *file path redacted*

What am I doing wrong here? :(

Answer 1

You declare BST as:

typedef struct node *BST;

So, it is a pointer to a struct node and your problem is probably here:

temp = *(BST *)malloc(sizeof(BST));

1. You are allocating memory but the byte size you specified is that of BST, that is pointer to struct node, while you want to allocate a struct node.

2. You cast the returned value of malloc to a pointer to a BST, that is, a pointer to a pointer to a struct node. And then you de-reference it to assign the result to temp. So, what you assign to temp is a pointer to a struct node (correct) but the pointed object has the wrong size. Anyway, you should not cast the value returned by malloc (void *).

Things are much simpler than you apparently think: if you want to allocate a struct node (or anything else) pass its size to malloc. malloc returns a void * pointer to the allocated struct node that you can assign to any pointer variable without casting.

Note: you should check the returned value because if malloc fails it returns NULL and you should not use that.

Try this, instead:

temp = malloc(sizeof (struct node));
if(temp == NULL) {
  fprintf(stderr, "%s:%d allocation failed\n", __FILE__, __LINE__);
  exit(EXIT_FAILURE);
}

I assumed that an allocation error, in your case, is unrecoverable, adapt if it is.

Note: you could also, as suggested in comments, use:

temp = malloc(sizeof *temp);

which will always work, by construction. But as long as you are not completely comfortable with pointers and memory allocation I suggest that you use explicit types with malloc. It is a bit easier to read and understand, even if it is a bit less easy to maintain (if you change the type of temp).

But there are other problems with your code. Your insert and find functions are bogus. Understanding why is left as a debugging exercise.

Answer 2

Main problem is with this statement:

temp = *(BST *)malloc(sizeof(BST));

As it was already explained in the other answer clearly, I am going to skip that.

So you can do:

temp = malloc(sizeof (struct node));

OR

// to add : this helps in case the type of temp ever changes,
temp = malloc(sizeof(*temp));

Other minor logical changes:

• In the find function, there is no need of this else if statement

    // = or == doesn't matter now
    else (root->data = number);

• In the insert function, you forgot to link the nodes which you insert after your root node. So whenever you perform the inorder traversal, you only get the root node i.e 0 in your case.

Note:

typedef is used to make code more readable, but hiding the pointer using typedef creates confusion. So I would suggest not to typedef pointers.