Segmentation fault (core dumped) error when making a linked list

Question

I'm learing C for school, and one of the assignments was to create a database. Now I'm trying to add some of the inputs I give it to the list, but I keep getting a segmentation error. What am I doing wrong?

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

struct carinfo_t {
    char* carbrand;
    char* carmodel;
    int caryear;
    float carvalue;
    struct carinfo_t * next;
};

struct carinfo_t * carbase;

struct carinfo_t * tempcar;

struct carinfo_t * tempcar2;

struct carinfo_t * tempprint;

void freeCarinfo(struct carinfo_t * carinfo){
    free(carinfo->carbrand);
    free(carinfo->carmodel);
    free(carinfo);    
}

struct carinfo_t * createCarinfo(char *carbrand, char *carmodel, int caryear, float carvalue){
    struct carinfo_t * newcar;
    newcar = (struct carinfo_t *)malloc(sizeof (struct carinfo_t));
    newcar->carbrand=(char *)malloc(sizeof(char)*(strlen(carbrand) + 1));
    strcpy(newcar->carbrand, carbrand);

    newcar->carmodel=(char *)malloc(sizeof(char)*(strlen(carmodel) + 1));
    strcpy(newcar->carmodel, carmodel);

    newcar->caryear=caryear;

    newcar->carvalue=carvalue;

    newcar->next= NULL;

    return newcar;
}

struct carinfo_t * addCarinfo(struct carinfo_t *carbase, struct carinfo_t *newcar){
    if(carbase=NULL){
        carbase = newcar;
        return carbase;
    }

    else{
        tempcar2->next=carbase;
        carbase=tempcar2;
        return carbase;

    }

}

void printCarbase(struct carinfo_t *carbase){

    struct carinfo_t *tempprint = carbase;

    if (carbase == NULL){
        printf("The database contains no cars\n");
    }

    else{
        while (tempprint != NULL){
            printf("Car:\n");
            printf("- brand: %s\n", carbase->carbrand);
            printf("- model: %s\n", carbase->carmodel);
            printf("- year: %d\n", carbase->caryear);
            printf("- value: %7.2f\n", carbase->carvalue);
            tempprint = tempprint->next;
        }
    }
}


void main(void){
    struct carinfo_t * carbase;
    carbase = NULL;

    struct carinfo_t * tempcar;
    tempcar = createCarinfo("Opel", "Manta", 1965, 20000);

    struct carinfo_t * tempcar2 = createCarinfo("Ford", "Focus", 1999, 350.25);
    addCarinfo(carbase, tempcar);
}

Also, if you see any way to improve my code please tell me, I'm very new to programming, and I'd like to be able to do this properly.

edit: Thanks to everyone who responded, I figured out how to use GDB. Now that the original issue is fixed, I got the same error but this time it's "tempcar2" that seems to be the issue:

Program received signal SIGSEGV, Segmentation fault.
0x000000000040072a in addCarinfo (carbase=0x602010, newcar=0x602080)
    at database.c:56
56              tempcar2 = tempcar2->next;
(gdb) bt
#0  0x000000000040072a in addCarinfo (carbase=0x602010, newcar=0x602080)
    at database.c:56
#1  0x0000000000400869 in main () at database.c:98

This sounds like the right time to learn how to use a debugger, so that you can step through the code yourself and see where it goes wrong — UnholySheep, Oct 24 '18 at 13:18
This article should interess you: https://ericlippert.com/2014/03/05/how-to-debug-small-programs/ — Mathieu, Oct 24 '18 at 13:18
`tempcar2` in `addCarinfo` is not initialized (it is still zero upon first use). — Paul Ogilvie, Oct 24 '18 at 13:19
Please compile your code with warnings enabled. The line `if(carbase=NULL)` should be a comparison with `==`, not a assignment with `=` and the compiler will warn you about it. — M Oehm, Oct 24 '18 at 13:19
Warning, `carbase` is a global variable and used as a local variable in many functions... — Mathieu, Oct 24 '18 at 13:25

danglingpointer · Answer 1 · 2018-10-24T13:40:26.373

I ran your program in gdb it points to the line 52

Program received signal SIGSEGV, Segmentation fault.
0x00000000004007b6 in addCarinfo (carbase=0x0, newcar=0x602010) at so.c:52
(gdb) bt 
#0  0x00000000004007b6 in addCarinfo (carbase=0x0, newcar=0x602010) at so.c:52
#1  0x00000000004008e7 in main () at so.c:89

Here its mistake, instead of == your using =.

// Your assigning carbase struct pointer to NULL instead of validating pointer is null. 
    if(carbase=NULL){ //change to carbase == NULL
        carbase = newcar;
        return carbase;
    }

How to debug using GDB:

compile your C code using -g flag if you use gcc. example: gcc -g file.c
gdb ./a.out
bt(backtrace if will pointout the callstack)

Mathieu · Accepted Answer · 2018-10-24T14:17:59.750

Some problems in your code:

some variable are not initialized (tempcar2 for instance, which is global)
some local and global variable have the same name
some comparison are done with = instead of == operator
in your print function, you always print carbase instead of tempprint
malloc return is casted, it should not

Some problems could have been detected by the compiler: turn on compiler warnings (-Wall for most compilers)

When your code is compiled with warning on, your problems are revealed:

.code.tio.c: In function ‘addCarinfo’:
.code.tio.c:46:8: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if(carbase=NULL){
        ^~~~~~~
.code.tio.c: At top level:
.code.tio.c:81:6: warning: return type of ‘main’ is not ‘int’ [-Wmain]
 void main(void){
      ^~~~
.code.tio.c: In function ‘main’:
.code.tio.c:88:24: warning: unused variable ‘tempcar2’ [-Wunused-variable]
    struct carinfo_t * tempcar2 = createCarinfo("Ford", "Focus", 1999, 350.25);
                        ^~~~~~~~

A corrected version could be:

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

struct carinfo_t
{
    char* carbrand;
    char* carmodel;
    int caryear;
    float carvalue;
    struct carinfo_t * next;
};

void freeCarinfo(struct carinfo_t * carinfo)
{
    free(carinfo->carbrand);
    free(carinfo->carmodel);
    free(carinfo);    
}

struct carinfo_t * createCarinfo(char *carbrand, char *carmodel, int caryear, float carvalue)
{
    struct carinfo_t * newcar;
    /* malloc cast is not recommender */
    newcar = malloc(sizeof (struct carinfo_t));

    /* strdup can be used here */
    newcar->carbrand = strdup(carbrand);    
    newcar->carmodel = strdup(carmodel);

    newcar->caryear=caryear;
    newcar->carvalue=carvalue;

    newcar->next= NULL;

    return newcar;
}

struct carinfo_t * addCarinfo(struct carinfo_t *carbase, struct carinfo_t *newcar)
{
    if(carbase==NULL)
    {
        carbase = newcar;
        return carbase;
    }
    else
    {
        /* find for the last element */
        struct carinfo_t * tempcar2 = carbase;
        while(tempcar2->next)
        {
            tempcar2 = tempcar2->next;
        }
        /* add the new car to the list */
        tempcar2->next=newcar;

        return carbase;
    }
}

void printCarbase(struct carinfo_t *carbase)
{
    struct carinfo_t *tempprint = carbase;

    if (carbase == NULL)
    {
        printf("The database contains no cars\n");
    }
    else
    {
        while (tempprint != NULL)
        {
            printf("Car:\n");
            printf("- brand: %s\n", tempprint->carbrand);
            printf("- model: %s\n", tempprint->carmodel);
            printf("- year: %d\n", tempprint->caryear);
            printf("- value: %7.2f\n", tempprint->carvalue);
            tempprint = tempprint->next;
        }
    }
}

int main(void)
{
   struct carinfo_t * carbase;
   carbase = NULL;

   struct carinfo_t * tempcar;
   tempcar = createCarinfo("Opel", "Manta", 1965, 20000);

   struct carinfo_t * tempcar2 = createCarinfo("Ford", "Focus", 1999, 350.25);
   carbase = addCarinfo(carbase, tempcar);
   carbase = addCarinfo(carbase, tempcar2);
   printCarbase(carbase);

   return 0;       
}

The result with this code is:

Car: 
- brand: Opel
- model: Manta
- year: 1965
- value: 20000.00
Car: 
- brand: Ford
- model: Focus
- year: 1999
- value:  350.25

In the createCarinfo function, you remove malloc 2 times, and instead use strdup. Is this to reduce clutter? Or does it have any other usage? Also, why does your version print the data for the Opel Manta twice, while also having added "tempcar2" to carbase? — Paul van Kleunen, Oct 24 '18 at 14:11
@PaulvanKleunen I use `strdup` because it does the `malloc` + `strcpy` in one pass, less code is often less bugs. — Mathieu, Oct 24 '18 at 14:19
@PaulvanKleunen Opel Manta is printed twice because of a bug I did not see in print `printCarbase` function, I updated my answer to explain it. — Mathieu, Oct 24 '18 at 14:20

Segmentation fault (core dumped) error when making a linked list

2 Answers2