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
– Iharob Al Asimi, Commented Jan 21, 2017 at 14:29
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, Commented Jan 21, 2017 at 14:35
@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
– tambre, Commented Jan 21, 2017 at 14:37
I uploaded all the info from Codeblocks debugger, hope it helps! Thanks! — ritgeo
– ritgeo, Commented Jan 21, 2017 at 15:00
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
– davmac, Commented Jan 21, 2017 at 15:07

chqrlie · Accepted Answer · 2017-01-21 16:42:55Z

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.

Thanks! I saw that the best thing to do is to use <0 or >0 for strcmp.. Also fixed the ; bug.. But why if i say.. if strcmp <0 do this..., else do something else doesnt work and gives still an infinite loop? and i have to define if strcmp>0?
@ritgeo: read my latest update. not returning the allocated root was causing an infinite recursion.

ritgeo · Accepted Answer · 2017-01-21 16:39:08Z

0

Ok, so after testing everything i found out what is wrong however i can't figure out why.. Any Suggestions?? I think that strcmp returns values of either 0 or -1 or 1. But there was the problem, if i didn't define exactly if strcmp ==1 or if strcmp ==-1 ...i had an infinite loop as it seems (i used a printf command to see what is going on, and i discovered the bug..)

By changing

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

to

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 if ((strcmp(l->data->email, root->data->data->email)) ==1)
        root->right =new_BST_node(root->right,l);

    return root;
};

everything works fine.. But why didnt the other code work? I am pretty sure it is the same...

edited Jan 21, 2017 at 16:39

answered Jan 21, 2017 at 16:06

ritgeo

718 bronze badges

3 Comments

Iharob Al Asimi Over a year ago

Breath, means l->data->email,root->data->data->email looks exactly like l->data->email->root->data->data->email also, chaining pointers like this is really bad. It's unreadable and can cause problems that no debugger will help you find.

ritgeo Over a year ago

Thanks for the advice Iharob! Fixed the solution to my problem, hope it is more readable than before..

Iharob Al Asimi Over a year ago

Also, you can't rely on strcmp()'s return value like that. Check != 0 instead.

Collectives™ on Stack Overflow

Segmentation fault while using malloc

2 Answers 2

2 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related