Segmentation fault while using malloc

Question

As an exercise I am trying to work on a Binary search tree!

I have created the code and it seems to run, but when I try to add a customer it crashes. After debugging the code I get a segmentation fault in line 82, where I try to allocate memory to root... Researching for a while I see that it is something related to memory, but can't figure what is going on with my code... Any suggestions about what is causing this failure when trying to allocate memory?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define MAX_STRING 50

void flush();

struct customer {
    char *name;
    char *address;
    char *email;
};

struct double_linked_list {
    struct customer *data;
    struct double_linked_list *previous;
    struct double_linked_list *next;
};

struct double_linked_list *customers_head = 0;

struct BST_node {
    struct double_linked_list *data;
    struct BST_node *left;
    struct BST_node *right;
};

struct BST_node *BST_email_root = 0;

struct BST_node *BST_find_customer(struct BST_node *root, char *email) {
    if (root == NULL)
        return NULL;
    if (strcmp(email, root->data->data->email) == 0)
        return root;
    else {
        if (strcmp(email, root->data->data->email) == -1)
            return BST_find_customer(root->left, email);
        else
            return BST_find_customer(root->right, email);
    }
}

void find_customer() {
    char email[MAX_STRING];
    struct double_linked_list *l;
    struct BST_node *b;

    printf("Give the email of the customer (up to %d characters) : ", MAX_STRING - 1);
    gets(email);

    b = BST_find_customer(BST_email_root, email);
    if (b == 0)
        printf("There is no customer with this email.\n");
    else {
        l = b->data;
        printf("Customer found! \n");
        printf("Name    : %s\n", l->data->name);
        printf("Address : %s\n", l->data->address);
        printf("Email   : %s\n", l->data->email);
    }
}

struct BST_node *new_BST_node(struct BST_node *root, struct double_linked_list *l) {
    if (root == NULL);
    {
        root = (struct BST_node *)malloc(sizeof(struct BST_node));
        if (root == NULL) {
            printf("Out of Memory!");
            exit(1);
        }
        root->data = l;
        root->left = NULL;
        root->right = NULL;
    }

    if (strcmp(l->data->email, root->data->data->email) == -1)
        root->left = new_BST_node(root->left, l);
    else 
        root->right = new_BST_node(root->right, l);
    return root;
};

struct double_linked_list *new_customer() {
    char name[MAX_STRING], address[MAX_STRING], email[MAX_STRING];
    struct BST_node *b;
    struct double_linked_list *l;
    struct customer *c;

    printf("\nADDING NEW CUSTOMER\n=\n\n");
    printf("Give name (up to %d characters): ", MAX_STRING - 1);
    gets(name);

    printf("Give address (up to %d characters): ", MAX_STRING - 1);
    gets(address);

    printf("Give email (up to %d characters): ", MAX_STRING - 1);
    gets(email);

    b = BST_find_customer(BST_email_root, email);
    if (b) {
        printf("Duplicate email. Customer aborted.\n");
        return 0;
    }

    c = (struct customer *)malloc(sizeof(struct customer));
    if (c == 0) {
        printf("Not enough memory.\n");
        return 0;
    }

    c->name = strdup(name); // check for memory allocation problem
    if (c->name == 0)
        return 0;
    c->address = strdup(address);   // check for memory allocation problem
    if (c->address == 0)
        return 0;
    c->email = strdup(email);   // check for memory allocation problem
    if (c->email == 0)
        return 0;

    l = (struct double_linked_list*)malloc(sizeof(struct double_linked_list));
    if (l == 0) {
        printf("Not enough memory.\n");
        free(c->name);
        free(c->address);
        free(c->email);
        free(c);
        return 0;
    }

    l->data = c;
    l->previous = 0;
    l->next = customers_head;

    if (customers_head)
        customers_head->previous = l;

    customers_head = l;

    BST_email_root = new_BST_node(BST_email_root, l);

    return l;
}

void displaymenu() {
    printf("\n\n");
    printf("1. New customer\n");
    printf("2. Find customer using email\n");
    printf("0. Exit\n\n");
    printf("Give a choice (0-6) : ");
}

void flush() {
    char ch;
    while ((ch = getchar()) != '\n' && ch != EOF);
}

int main() {
    int choice;
    do {
        displaymenu();
        scanf("%d", &choice);
        flush();
        switch (choice) {
          case 1:
            new_customer();
            break;
          case 2:
            find_customer();
            break;
        }
    } while (choice != 0);

    return 0;
}

Codeblocks debugger gives the following information

Active debugger config: GDB/CDB debugger:Default

Building to ensure sources are up-to-date

Selecting target:

Debug Adding source dir: C:\debug\

Adding source dir: C:\debug\

Adding file: C:\debug\bin\Debug\debug.exe

Changing directory to: C:/debug/.

Set variable: PATH=.;C:\Program Files\CodeBlocks\MinGW\bin;C:\Program Files\CodeBlocks\MinGW;C:\Windows\System32;C:\Windows;C:\Windows\System32\wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Program Files\ATI Technologies\ATI.ACE\Core-Static;C:\Users\baskon\AppData\Local\Microsoft\WindowsApps

Starting debugger: C:\Program Files\CodeBlocks\MINGW\bin\gdb.exe -nx -fullname -quiet -args C:/debug/bin/Debug/debug.exe

done

Registered new type: wxString

Registered new type: STL String

Registered new type: STL Vector

Setting breakpoints

Debugger name and version: GNU gdb (GDB) 7.6.1

Child process PID: 5908

Program received signal SIGSEGV, Segmentation fault.

In ?? () ()

9 0x00401480 in new_BST_node (root=0x0, l=0xbe0ec8) at C:\debug\main.c:82 C:\debug\main.c:82:1907:beg:0x401480

At C:\debug\main.c:82

9 0x00401480 in new_BST_node (root=0x0, l=0xbe0ec8) at C:\debug\main.c:82 C:\debug\main.c:82:1907:beg:0x401480

The call stack is the following

Segmentation Fault, never happens when using malloc(). Also, let your code breath. — Iharob Al Asimi
Thanks for your suggestions, i think now my code is more readable. This is what the debugger is suggesting though.. Segmentation fault in line 82, where i allocate memory! — ritgeo
@ritgeo Could you please include the call stack? Also, before debugging make sure to generate debug information to make debugging easier (-g flag on GCC). — tambre
I uploaded all the info from Codeblocks debugger, hope it helps! Thanks! — ritgeo
Please fix your indentation. The code is difficult to read. Also, is this really the minimal code that will produce the problem? See minimal reproducible example. — davmac

chqrlie chqrlie · Accepted Answer · 2017-01-21T16:34:28

There are multiple problems in your code:

You have a classic bug here:
```
struct BST_node *new_BST_node(struct BST_node *root, struct double_linked_list *l) {
if (root == NULL);
{
    root = (struct BST_node *)malloc(sizeof(struct BST_node));
```
The ; at the end of the if statement line is parsed as an empty statement. The subsequent block, introduced by the { will always be executed.

You can avoid this kind of silly bug by always using braces for your compound statements and placing them on the same line as the beginning of the statement, not on the next line. This is close to the original K&R style used by the creators of the C language more than 40 years ago.
The type for variable ch in function flush should be int to allow proper distinction of all values returned by getc(): all values of unsigned char plus the special value EOF:
```
 void flush(void) {
    int ch;
    while ((ch = getchar()) != EOF && ch != '\n') {
        continue;
    }
}
```
Note that you should make the empty statement more explicit as I did above, to avoid confusion and bugs such as the previous one.
You should not use the obsolete unsafe function gets(): Use fgets() and strip the trailing newline with strcspn().
When comparing strings with strcmp(), you should only rely on the sign of the return value. In functions BST_find_customer() and new_BST_node(), you compare with -1: this is unreliable. Use if (strcmp(email, root->data->data->email) < 0) instead.

In function new_BST_node(), You do not return root when you create a new node in the tree. Here is a corrected version:

struct BST_node *new_BST_node(struct BST_node *root, struct double_linked_list *l) {
    if (root == NULL) {
        root = malloc(sizeof(*root));
        if (root == NULL) {
            printf("Out of Memory!");
            exit(1);
        }
        root->data = l;
        root->left = NULL;
        root->right = NULL;
        return root;
    }

    if (strcmp(l->data->email, root->data->data->email) < 0) {
        root->left = new_BST_node(root->left, l);
    } else {
        root->right = new_BST_node(root->right, l);
    }
    return root;
}

Note that this is causing your bug as you probably have an infinite recursion here.

Finally a couple pieces of advice:
- use NULL instead of 0 when comparing pointers to the null pointer. It is more readable.
- avoid naming a variable l as this name is graphically too close to 1 with the fixed pitch fonts used in programming environments. Sometimes it is almost indistinguishable.

Segmentation fault while using malloc

2 Answers