Segmentation fault using structs in C

Question

I'm testing out what we recently learned in class about structs and pointers by writing a small C program. However, after running it i came across a segmentation fault (core dumped) error. Can someone help me figure out where exactly that's caused? Did i miss any dangling pointers or did i do something wrong using malloc()?

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

const char *admin = "Administrator";
const char *mng = "Manager";
const char *prog = "Programmer";

struct employee {
    char *name;
    char title[20];
    char id[8];
    int yearsExp;
};

typedef struct employee emp_t;

emp_t *everyone[1];

emp_t *create_emp(const char *name,const char *title,const char *id,int yrxp) {
    emp_t *new;

    new = (emp_t *) malloc(sizeof(emp_t));
    new->name = (char*) malloc(strlen(name) + 1);
    strcpy(new->name,name);
    strcpy(new->title,title);
    strcpy(new->id,id);
    new->yearsExp = yrxp;
    return new;
}

void free_emp(emp_t *employee) {
    free(employee->name);
    free(employee);
}

int main() {
    int i;

    everyone[0] = create_emp("Mike Papamichail",prog,"A197482",3);
    everyone[1] = create_emp("Maria Mamalaki",mng,"Z104781",6);

    for(i = 0; i < 2;i++) {
        printf("%15s \t %15s \t %10s \t %4d\n",
        everyone[0]->name,everyone[0]->title,everyone[0]->id,everyone[0]->yearsExp);
        free_emp(everyone[i]);
        everyone[i] = NULL;
    }
    return 0;
}

Updated Code for clarity

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

const char *admin = "Administrator";
const char *mng = "Manager";
const char *prog = "Programmer";

struct employee {
    char *name;
    char title[20];
    char id[8];
    int yearsExp;
};

typedef struct employee emp_t;

emp_t *create_emp(const char *name,const char *title,const char *id,int yrxp) {
    emp_t *new;

    new = (emp_t *) malloc(sizeof(emp_t));
    new->name = (char*) malloc(strlen(name) + 1);
    strcpy(new->name,name);
    strcpy(new->title,title);
    strcpy(new->id,id);
    new->yearsExp = yrxp;
    return new;
}

void free_emp(emp_t *employee) {
    free(employee->name);
    free(employee);
}

int main() {
    int i;
    emp_t *everyone[2];

    everyone[0] = create_emp("Mike Papamichail",prog,"A197482",3);
    everyone[1] = create_emp("Maria Mamalaki",mng,"Z104781",6);

    for(i = 0; i < 2;i++) {
        printf("%15s \t %15s \t %10s \t %4d\n",
        everyone[0]->name,everyone[0]->title,everyone[0]->id,everyone[0]->yearsExp);
        free_emp(everyone[i]);
        everyone[i] = NULL;
    }
    return 0;
}

Hint: emp_t *everyone[1] creates an array of length 1, so the maximum index is 0. everyone[1] is out of range. — tadman
– tadman, Commented Dec 28, 2018 at 22:50
@RobertHarvey It's C. You can do anything, especially that. C presumes you know what you're doing when you ask to overrun something, so it's up to you as the programmer to not do that if you don't want undefined behaviour. The reason I suggest dynamically allocating is that way the size declaration and the population code exist side-by-side so mistakes are more obvious. — tadman
– tadman, Commented Dec 28, 2018 at 22:57
@tadman you are right about the first part, i already changed it in my code. As for the second part, i'll apply the necessary changes to my code asap! — Stelios Papamichail
– Stelios Papamichail, Commented Dec 28, 2018 at 23:01
Why are you calling malloc on the array, and then throwing those away when calling create_emp right on top? A habit you need to ditch is casting your malloc result. It's not necessary. Let the compiler deal with that. — tadman
– tadman, Commented Dec 28, 2018 at 23:14
Yeah, well, sadly there's a long tradition of computer science professors saying things that simply aren't true. What specific thing do you need explained better? — tadman
– tadman, Commented Dec 28, 2018 at 23:22

David C. Rankin · Accepted Answer · 2018-12-28 23:44:35Z

You are close on all points @tadman caught your biggest error in creating an array of 1 pointer with emp_t *everyone[1] (now deleted in your question).

The remainder of the issues with your code are more a number of small corrections or improvement comparatively speaking.

For starters, avoid using "magic-numbers" in your code (e.g. 20, 8, 2). If you need constants, #define them, or use a global enum to define multiple constants at once, e.g.

/* an enum can be used to #define multiple constants */
enum { MAXID = 8, MAXTITLE = 20, MAXEMP = 128 };

and then

typedef struct {    
    char *name;
    char title[MAXTITLE];   /* avoid "magic-numbers", use constants */
    char id[MAXID];
    int yearsExp;
} emp_t;

Your create_emp() function will largely work as is, but there is no need to cast the return of malloc, calloc or realloc in C, see: Do I cast the result of malloc?. Also, I would avoid the use of new as a temporary struct name. While there is no actual conflict in C, new is a keyword in C++ and if you will code in both, best to keep that in mind. With a couple of tweaks, you would write create_emp() as follows:

emp_t *create_emp (const char *name, const char *title, 
                    const char *id, int yrxp) {
    emp_t *newemp;  /* new is a keyword in C++ best to avoid */

    newemp = malloc (sizeof *newemp);   /* don't cast return of malloc */
    /* if you allocate, you must validate */
    if (newemp == NULL) {
        perror ("malloc-newemp");
        return NULL;
    }
    newemp->name = malloc (strlen(name) + 1);
    if (newemp->name == NULL) {
        perror ("malloc-newemp->name");
        free (newemp);
        return NULL;
    }
    strcpy (newemp->name, name);
    strcpy (newemp->title, title);
    strcpy (newemp->id, id);
    newemp->yearsExp = yrxp;

    return newemp;
}

(note: always validate each allocation. malloc, calloc & realloc can and do fail)

Lastly, in main(), you can use an index (ndx below) instead of the magic-number 2 and increment the index with each addition. While you are doing well to use field-width modifiers to control your output field size, for strings (and all conversion specifiers) you can include the '-' flag as part of the conversion specifier to make the field Left-Justified to align your output properly. The remainder of your code is fine.

Putting it altogether, you could do something like the following:

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

/* an enum can be used to #define multiple constants */
enum { MAXID = 8, MAXTITLE = 20, MAXEMP = 128 };
#define ADMIN   "Administrator" /* simple #defines are fine */
#define MNG     "Manager"       /* string literals are fine as well */
#define PROG    "Programmer"    /* (up to you) */

typedef struct {    
    char *name;
    char title[MAXTITLE];   /* avoid "magic-numbers", use constants */
    char id[MAXID];
    int yearsExp;
} emp_t;

emp_t *create_emp (const char *name, const char *title, 
                    const char *id, int yrxp) {
    emp_t *newemp;  /* new is a keyword in C++ best to avoid */

    newemp = malloc (sizeof *newemp);   /* don't cast return of malloc */
    /* if you allocate, you must validate */
    if (newemp == NULL) {
        perror ("malloc-newemp");
        return NULL;
    }
    newemp->name = malloc (strlen(name) + 1);
    if (newemp->name == NULL) {
        perror ("malloc-newemp->name");
        free (newemp);
        return NULL;
    }
    strcpy (newemp->name, name);
    strcpy (newemp->title, title);
    strcpy (newemp->id, id);
    newemp->yearsExp = yrxp;

    return newemp;
}

void free_emp (emp_t *employee) {
    free (employee->name);
    free (employee);
}

int main (void) {

    int i, ndx = 0;     /* use an index instead of magic-numbers */

    emp_t *everyone[MAXEMP] = {NULL};

    everyone[ndx++] = create_emp ("Mike Papamichail", PROG, "A197482", 3);
    everyone[ndx++] = create_emp ("Maria Mamalaki", MNG, "Z104781", 6);
    everyone[ndx++] = create_emp ("Sam Sunami", ADMIN, "B426310", 10);

    for (i = 0; i < ndx; i++) { /* %- to left justify fields */
        if (everyone[i]) {      /* validate not NULL */
            printf ("%-15s \t %-15s \t %-10s \t %4d\n", everyone[i]->name, 
                    everyone[i]->title, everyone[i]->id, 
                    everyone[i]->yearsExp);
            free_emp (everyone[i]);
            everyone[i] = NULL;
        }
    }

    return 0;
}

(note: the change from everyone[0]->name to everyone[i]->name, etc.., otherwise the output never changes)

Example Use/Output

$ ./bin/emp_struct
Mike Papamichail         Programmer              A197482            3
Maria Mamalaki           Manager                 Z104781            6
Sam Sunami               Administrator           B426310           10

Look things over and let me know if you have further questions.

Thank you so much for the answer, since it's too late here i will review it in the morning and post here again, hope you don't mind. Thanks again!
No problem, note: I added additional validations for malloc and then within the output loop (which you would usually do in your fill loop, but that is hardcoded here)
Thank you so much for the answer and the explanations.I finally understand and hopefully i will pick up some of those best practices displayed here. One last question if you don't mind, what does left-justified mean?
That means the names are displayed to the left of the field, e.g. "Maria Mamalaki " instead of " Maria Mamalaki". (the default, without the '-' is Right-Justified where the words are aligned to the right end of the field)

Chris Dodd · Accepted Answer · 2018-12-28 23:58:21Z

0

At the end, you're looping through your array, printing everyone[0] and then freeing everyone[i]. So on the second iteration, everyone[0] will be null and the code crashes...

answered Dec 28, 2018 at 23:58

Chris Dodd

127k14 gold badges150 silver badges243 bronze badges

Collectives™ on Stack Overflow

Segmentation fault using structs in C

2 Answers 2

4 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related