Segmentation faults for allocating memory for a binary search tree in C

Question

I am trying to implement a binary search tree in C and am getting a segmentation fault when trying to run my code. Essentially in my code I am reading in a line of data from a file, creating a node for it and then inserting that into my binary search tree. I am not even sure if my implementation for this code is correct as I'm getting a segmentation fault when I try to run it. I'm very new to C programming and especially allocating memory so I am aware there are probably a large amount of errors in my code, even in the core structure of the code itself. As such any help with finding the reason behind the segmentation fault or help with the core structure of the code itself would be appreciated.

Answer 1

This is maybe more of a comment than an answer, but I don't have enough "reputation" to comment, so...

The problem has to do with the use, and not, of pointers, as commented by @Lundin earlier.

In this line: *node->left = insertNode(node->left, newNode);

node->left can be NULL. Inside the call to insertNode, you check for this and assign newNode to the local copy of node->left, but that has no effect on node->left, so when the return value of insertNode is written to the location pointed to by node-left (NULL), you have a guaranteed crash!

Answer 2

You are free'ing a pointer to your stack :

int main(int argc, char *argv[]) {

bst_t root;                                       <<< root is allocated on stack
bst_t newNode;
int c;

while ((c = getchar()) != EOF){
  newNode = createNode();
  root = insertNode(&root,&newNode);
  }
  freeTree(&root);                                <<< Your are passing a pointer to root ( pointer to a stack element )
  return 0;
}



void freeTree(bst_t *parent){                    <<< parent is a pointer to a stack element
  if(! parent){
    return;
  }
  freeTree(parent->left);
  freeTree(parent->right);
  free(parent);                                  <<< you free a stack element => segmentation fault
}

You either have to allocated the root node so it can be free'ed or handle the root node in your freeTree

---

EDIT : Here is the syntax to malloc it :

bst_t *root = malloc(sizeof(bst_t));

malloc function returns a pointer to a newly allocated space, and takes as parameter the size of the allocation.

side note : If you wanna be really clean about it, malloc can return null if there is no memory available ( very rare ), so it is a good practice to check the return.
more info

Answer 3

I believe the problem is in the readData function. When you use scanf, you have to use the address-of operator & to read into variables.

The following code:

void readData(bst_t *node) {
while (scanf("%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],"
             "%[^,],%[^,],%[^,],%[^,],%[^,]",
             node->data.ID, node->data.name, node->data.sex,
             node->data.height, node->data.weight, node->data.team,
             node->data.NOC, node->data.games, node->data.year,
             node->data.season, node->data.city, node->data.sport,
             node->data.event, node->data.medal) == 14) {
    continue;
}

should be changed to:

void readData(bst_t *node) {
while (scanf("%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],"
             "%[^,],%[^,],%[^,],%[^,],%[^,]",
             &node->data.ID, &node->data.name, &node->data.sex,
             &node->data.height, &node->data.weight, &node->data.team,
             &node->data.NOC, &node->data.games, &node->data.year,
             &node->data.season, &node->data.city, &node->data.sport,
             &node->data.event, &node->data.medal) == 14) {
    continue;
}

Also some modifications you should make:
1. Pass arguments and return values using pointers to structures rather than structures.
2. Allocate memory on the heap and not on the stack when dealing with linked lists (Even for the root node).
3. When allocating memory on the heap, make sure that the size allocated is the size of the structure and not the size of pointer to structure.
4. Make sure you free all the memory allocated on the heap and don't mistakenly free the memory allocated on the stack as you seem to be doing here.

Answer 4

There are multiple problems in your code:

• root is uninitialized in the main function. It must be initialized to NULL before passing it to insertNode().

• you should only parse a single line in readdata() and return a success indicator.

• you should iterate in the main loop until readdata fails to read more records from the file. You current test while ((c = getchar()) != EOF) is inappropriate and corrupts this input stream.

Here are modified versions of the readdata and main functions:

bst_t *createNode(void) {
    bst_t *newNode;
    newNode = (struct bst_t *)malloc(sizeof(struct bst_t));
    newNode->left = NULL;
    newNode->right = NULL;
    return newNode;
}

int readData(bst_t *node) {
    return scanf("%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],%[^,],"
                 "%[^,],%[^,],%[^,],%[^,],%[^,]",
                 node->data.ID, node->data.name, node->data.sex,
                 node->data.height, node->data.weight, node->data.team,
                 node->data.NOC, node->data.games, node->data.year,
                 node->data.season, node->data.city, node->data.sport,
                 node->data.event, node->data.medal) == 14;
}

int main(int argc, char *argv[]) {
    bst_t *root = NULL;
    bst_t *newNode;

    while ((newNode = createNode()) != NULL) {
        if (readData(newNode)) {
            /* record was read correctly: insert into the tree */
            root = insertNode(root, newNode);
        } else {
            /* end of file or input error: stop reading the file */
            free(newNode);
            break;
        }
    }
    freeTree(root);
    return 0;
}