0

I am having problems with segmentation fault. When I run this particular code on a windows machine with devc++ or codeblocks it runs how I intended. The problem occurs when I try and run on a linux machine. I was told that it was most likely due to pointers trying to access locations they are not allowed but i cannot figure out why.

When I run the program without the "empinfo.txt" file it will start and allow me to do any of the menu options except for "1-Add", so the problem seems to have something to do with the way I am using arrays with my structure.

I included all of the code but the only problems functions(i think) are initialize and add. Any help on what is the correct way to use an array of chars in this case would be highly appreciated.

Here is sample input that would be in empinfo.txt file

12   JackSprat   2     1    65000
13   HumptyDumpty  5   3    30000
17   BoPeep  2       3      30000
20   BoyBlue    3    2      58000
0

-

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

FILE *empinfo=NULL;

struct employeeData {
    int EMP_ID;
    char name[20];
    int dept;
    int rank;
    double salary;
    struct employeeData *next;
};

struct employeeData *head = NULL;

void initializeList();
void add(int ID, char name[0], int dept, int rank, double newSalary);
void deleteEmp(int ID);
void modify(int ID, double NewSalary);
void query(int rank);
void print();

int main(){

    int choice=1, ID=0, rank=0, dept=0;
    double newSalary=0;
    char name[20];
    initializeList();

    while (choice != 0) {
        printf("1-Add 2-Delete 3-Modify 4-Query 5-Print 0-Exit\n");
        fflush(stdin);
        scanf("%d", &choice);

        if (choice == 1) {
            printf("Enter new employee info. Format: \"ID name dept rank salary\"\n");
            if((scanf("%d %s %d %d %lf", &ID, name, &dept, &rank, &newSalary))==5)
                add(ID, name, dept, rank, newSalary);
            else{
                printf("invalid format entered\n\n");
                continue;
            }
        }
        else if (choice == 2){
            printf("Enter employee ID to delete: ");
            if(scanf("%d", &ID)==1){
                deleteEmp(ID);}
            else{
                printf("Employee ID must be an integer\n");
                continue;
            }
        }
        else if (choice == 3){
            printf("Enter employee ID to modify: ");
            if(scanf("%d", &ID)==1){
                printf("Enter new salary amount: ");
                if(scanf("%lf", &newSalary)==1)
                    modify(ID,newSalary);
                else
                    printf("Salary must be a number\n");
            }
            else{
                printf("Employee ID must be an integer\n");
                continue;
            }
        }
        else if (choice == 4){
            printf("Enter the rank you wish to query: ");
            scanf("%d", &rank);
            query(rank);
        }
        else if (choice == 5){
            print();
        }
    }
    printf("Goodbye...\n");
    head=NULL;
    free(head);
    return 0;
}

void initializeList(){


    empinfo=fopen("empinfo.txt", "r");
    head = (struct employeeData *)malloc(sizeof(struct employeeData));
    head->next = NULL;
    if (empinfo==NULL){
        printf("empinfo.txt not found, file not opened.\n");
        head=NULL;
        free(head);
        free(empinfo);
        return;
    }

    struct employeeData *tempPtr = head;


    while (tempPtr->EMP_ID != 0){
        fscanf(empinfo, "%d %s %d %d %lf", &tempPtr->EMP_ID, tempPtr->name, &tempPtr->dept, &tempPtr->rank, &tempPtr->salary);
        if (tempPtr->EMP_ID == 0){
            break;
            }
        tempPtr->next = (struct employeeData *)malloc(sizeof(struct employeeData));
        tempPtr=tempPtr->next;
    }
    tempPtr=head;
    while(tempPtr->next->EMP_ID!=0){
        tempPtr=tempPtr->next;
    }
    empinfo=NULL;
    free(empinfo);
    tempPtr->next=NULL;
    fclose(empinfo);
    tempPtr=NULL;
    free(tempPtr);
}

void add(int ID, char name[], int dept, int rank, double newSalary){
    struct employeeData *tempPtr = head;
    while ((tempPtr->next!=NULL) && (tempPtr->next->EMP_ID < ID)) {
        tempPtr=tempPtr->next;
    }
    struct employeeData *newNode = (struct employeeData * )malloc(sizeof(struct employeeData));
    newNode->EMP_ID = ID;
    strcpy(newNode->name,name);
    newNode->dept = dept;
    newNode->rank = rank;
    newNode->salary = newSalary;
    newNode->next=NULL;
    if (tempPtr==head) {
        if(ID>tempPtr->EMP_ID){
            newNode->next = tempPtr->next;
            tempPtr->next = newNode;
        }
        else {
            newNode->next=tempPtr;
            head=newNode;
        }
    }
    else if (tempPtr->next == NULL){
        tempPtr->next=newNode;
        newNode->next=NULL;
    }
    else{
        newNode->next = tempPtr->next;
        tempPtr->next = newNode;
    }
    printf("Employee #%d has been added to the database.\n\n", ID);
    tempPtr=NULL;
    newNode=NULL;
    free(newNode);
    free(tempPtr);

}
void deleteEmp(int ID){
    struct employeeData *seek=head,*tempPtr = head;

    if(head!=NULL) {
        while((tempPtr->EMP_ID!=ID)){
                seek = tempPtr;
                tempPtr=tempPtr->next;
                if(tempPtr==NULL){
                    printf("Employee ID not found\n");
                    free(tempPtr);
                    seek=NULL;
                    free(seek);
                    return;
                }
        }
        if (tempPtr!=NULL){
            if(tempPtr==head){
                if(tempPtr->next==NULL){
                    printf("List cannot be empty\n");
                    free(tempPtr);
                    seek=NULL;
                    free(seek);
                    return;
                }
                head=tempPtr->next;
                free(tempPtr);
            }
            else if (tempPtr->next==NULL){
                free(tempPtr);
                seek->next = NULL;
            }
            else{
                seek->next=tempPtr->next;
                free(tempPtr);
            }
        }
    }
    printf("Employee #%d has been deleted\n", ID);
    tempPtr=NULL;
    free(tempPtr);
    seek=NULL;
    free(seek);
}

void modify(int ID, double NewSalary){
    struct employeeData *tempPtr = head;

    while (tempPtr!=NULL&&tempPtr->EMP_ID!=ID){
        tempPtr=tempPtr->next;
    }
    if(tempPtr==NULL){
        printf("Employee ID not found\n");
        free(tempPtr);
        return;
    }
    tempPtr->salary=NewSalary;
    printf("Employee salary updated.\n\n");
    tempPtr=NULL;
    free(tempPtr);
}

void query(int rank){
    struct employeeData *tempPtr = head;

     while (tempPtr!=NULL){
        if(tempPtr->rank == rank){
            printf("%s\n", tempPtr->name);
            tempPtr=tempPtr->next;
        }
        else
            tempPtr=tempPtr->next;
     }
     tempPtr=NULL;
     free(tempPtr);
}

void print(){
    struct employeeData *tempPtr = head;

    while (tempPtr!=NULL){
        printf("%d %s %d %d %.0lf\n", tempPtr->EMP_ID, tempPtr->name, tempPtr->dept, tempPtr->rank, tempPtr->salary);
        tempPtr=tempPtr->next;
    }
    free(tempPtr);
  • The continues are unnecessary; the code in the `if` block before each continue ends up going automatically to the same place as the code in the else block with the continue. However, that has nothing to do with segmentation faults. – Jonathan Leffler Sep 24 '14 at 16:26
  • In `initializeList()`, it would be simpler to defer the memory allocation until after you've checked that you've opened the file successfully; there'd be less clean up to do on the error path. – Jonathan Leffler Sep 24 '14 at 16:28
  • Ok I did that and it still segmentation faults, so it is accessing the file i assume. – Daniel Kinney Sep 24 '14 at 16:33
  • Why are you calling `free(NULL)` literally everywhere? It does literally nothing. – Bill Lynch Sep 24 '14 at 16:35
  • I've only scanned the code quickly and haven't worked out what your problem is. I think you pre-allocate the node in the list before you know whether you'll need it, which is probably not a good strategy. However, I've not understood it all. My prior suggestions were primarily cosmetic - one of the reasons they were comments, not in an answer. – Jonathan Leffler Sep 24 '14 at 16:35
  • I wanted to make sure every single pointer i had was freed and set to NULL to try and narrow down what the problem was. My actual code doesn't have half of the stuff in here, I just finally narrowed it down to the fscanf array read. – Daniel Kinney Sep 24 '14 at 16:37
  • After a quick look I see two things you are doing wrong. 1. You are setting pointers to NULL before you free them. The free function needs to get the actual pointer value before it can free the memory it points to. If you do this. p=NULL; free(p); then free gets a NULL pointer. 2. DO NOT free the file pointer you get from fopen. fopen takes care of allocated memory and fclose takes care of freeing it. If you like you can set the file pointer to NULL AFTER you call fcloase. – Stuart Sep 24 '14 at 16:42

2 Answers2

2

Use a debugger. If you are on Linux you probably have access to gdb. Here is a session with your example and the provided test file:

$ gdb test
...
(gdb) run
...
Program received signal SIGSEGV, Segmentation fault.
0x0000000000400bb2 in initializeList () at test.c:111
111     while(tempPtr->next->EMP_ID!=0){
Missing separate debuginfos, use: debuginfo-install glibc-2.18-14.fc20.x86_64
(gdb) p tempPtr
$1 = (struct employeeData *) 0x603250
(gdb) p tempPtr->next
$2 = (struct employeeData *) 0x0

We can see that we are trying to dereference a NULL pointer on line 111. It should be easy to locate the problem with this information.

This is also sure to cause errors:

empinfo=fopen(...)
...
empinfo=NULL;
free(empinfo);
fclose(empinfo);

You should not free the handle you got from fopen and you should not set it to NULL, let fclose handle this.

megahallon
  • 144
  • 3
  • Thank's for the help I'll try to figure it out from this info. I wasn't aware i could use the debugger while connecting to Linux server form windows machine. Thanks – Daniel Kinney Sep 24 '14 at 16:42
0

Welcome to the world of undefined behaviour! Beside a lot of other things, UB happens if you do this:

The value of the object allocated by the malloc function is used (7.20.3.3).

Find out more in the C99 standard, specifically take a look at Annex J. This is not a fun read, but essential if you want to use the C language in any other context than playing around.

So what happened to you is that on the Windows platform the memory returned by malloc() was obviously initialized to non-zero, including tempPtr->EMP_ID. That's why it worked there, but since it still is undefined behaviour, it is still wrong! On Linux the memory was obviously set to all-0 leading to the while() loop in initializeList() being skipped, resulting in a dereference of tempPtr->next which was set to NULL just a few lines before...

Since there're a lot of things wrong in your code, here're a few tips:

  • Use calloc() instead of malloc(). This initializes the memory to all-0
  • Do not cast the result of malloc (did this by myself for long time, but got convinced by the linked thread)
  • Check the return result of all your function calls, specifically memory allocations!
  • Set all pointer members of allocated structs manually to NULL
  • Initialize all variables before you use them! This may include memset()ing arrays
  • Read the documentation of fscanf(). Never used it by myself, but your use it not secure (buffer overflows)
  • Always read the documentation of system and library functions, especially the part where it comes to error handling. Currently, you do no error handling at all.
  • Ensure every use of free() makes sense. Just putting it everywhere where you think it may be wise is not a good coding practice :). Using it for file descriptors is just wrong.
  • Seriously consider another language. It's sad to see so many lines of code for such a task. C has its strengths, your case does not play them at all.
Community
  • 1
  • 1
grasbueschel
  • 879
  • 2
  • 8
  • 24