Why does this array of structure only access the first element & shows segmentation fault for the rest of the elements?

Question

I've spent three hours trying to figure out why I'm getting a segmentation fault in this particular case & I went through a lot of questions on this topic but still I couldn't relate it to my case.

Now I seem to know what is the problem but I don't know why.

My array of structures doesn't seem to be initialized properly.

All the values which I take as input from the user are displayed properly in the main function but when I go inside the calc_bonus(..) function, it runs correct for i=0, but gives segmentation fault for i=1 & there on.

This was a simple assignment question which I solved successfully( got all the test cases right using a naive approach) but I want to know where my main solution went wrong. Here's the initial code where I'm trying to find out netsalary & bonus.

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

struct Employee
{
    char empid[50];
    int basicsalary;
    int pf;
    int mediclaim;
    float salespercentage;
    int bonus;
    float netsalary;
};

int calc_NetSalary(struct Employee**,int);
int calc_Bonus(struct Employee**,int);

int main()
{
    int n,i;
    puts("Enter total records");
    scanf("%d",&n);
    struct Employee *emp = malloc(n*sizeof(*emp));
    int flag[n];
    for(i=0;i<n;i++)
    {
        puts("Enter the employee id");
        scanf(" %[^\n]s",emp[i].empid);
        puts("Enter the basic salary");
        scanf("%d",&emp[i].basicsalary);
        puts("Enter the PF amount");
        scanf("%d",&emp[i].pf);
        puts("Enter the mediclaim amount");
        scanf("%d",&emp[i].mediclaim);
        puts("Enter the sales percentage");
        scanf("%f",&emp[i].salespercentage);
        if(emp[i].basicsalary<0||emp[i].pf<0||emp[i].mediclaim<0||emp[i].salespercentage<0)
            flag[i]=1;
        else
            flag[i]=0;
        emp[i].bonus=calc_Bonus(&emp,i);
        emp[i].netsalary=calc_NetSalary(&emp,i);
    }
    for(i=0;i<n;i++)
    {
        if(flag[i])
            printf("Unable to calculate salary for the ID %s\n",emp[i].empid);
        else
           printf("Net salary for the ID %s is Rs.%.2f\n",emp[i].empid,emp[i].netsalary);
    }
    free(emp);
}

int calc_Bonus(struct Employee **emp,int i)
{

    if(emp[i]->basicsalary<=7000 && emp[i]->salespercentage<=10)
        return 1500;
    else if(emp[i]->basicsalary<=7000 && emp[i]->salespercentage>=10)
        return 3000;
    else if(emp[i]->basicsalary<=15000 && emp[i]->basicsalary>7000 && emp[i]->salespercentage<=10)
        return 2000;
    else if(emp[i]->basicsalary<=15000 && emp[i]->basicsalary>7000 && emp[i]->salespercentage>=10)
        return 4000;
    else if(emp[i]->basicsalary>15000 && emp[i]->salespercentage<=10)
        return 2500;    
    else if(emp[i]->basicsalary>15000 && emp[i]->salespercentage>=10)
        return 4500;

}

int calc_NetSalary(struct Employee **emp,int i)
{
    int a=emp[i]->basicsalary-emp[i]->pf-emp[i]->mediclaim+emp[i]->bonus;
    return a;
}

Now I've tried to debug it by printing out the values of bonus & netsalary for different iterations & it works only for i=0 & gives 12 segmentation fault core dump.

Can anyone point out exactly what I'm doing wrong here?

I checked to see that emp variables had been initialised though the main problem starts at the second iteration when I call the calc_Bonus(..) function. The value of emp[1].basicsalary gives that error but I don't know why.

EDIT: What I did & got as output compared to what should be the output.

Enter total records
2
Enter the employee id
428
Enter the basic salary
5500
Enter the PF amount
550
Enter the mediclaim amount
1203
Enter the sales percentage
8.5
Enter the employee id
430
Enter the basic salary
12000
Enter the PF amount
350
Enter the mediclaim amount
650
Enter the sales percentage
10.5
/home/p10301/.vpl_launcher.sh: line 12: 11946 Segmentation fault        (core dumped) ./vpl_execution

What the expected output should be:

Net salary for the ID 428 is Rs.5247.00
Net salary for the ID 430 is Rs.15000.00

Answer 1

You are allocating a sequence of struct Employee objects (i.e. malloc(n * sizeof(struct Employee)), but in function calc_bonus you treat them as an array of pointers to such struct Employee objects (and you pass a pointer to such a sequence of pointers, i.e. struct Employee**). Hence, in function calc_bonus, when you write emp[i], then you get back the ith pointer in emp. But emp does not contain pointers, it contains the employee objects. So emp[i] will return a value that is treated as a pointer, but the pointer value will point to something arbitrary, which is very likely on a struct Employee-object. And dereferencing such an "invalid" pointer yields undefined behaviour, which might become visible as a segmentation fault.

To overcome this, define and use calc_Bonus as follows:

int calc_Bonus(struct Employee[],int);

int calc_Bonus(struct Employee emp[],int i)
{
    if(emp[i].basicsalary<=7000 && emp[i].salespercentage<=10) {
    ...
}

int main () {
  ...
  emp[i].bonus=calc_Bonus(emp,i);
}

Answer 2

Your calc_Bonus() method should not have the array and index number as parameters. It should just have one parameter - a pointer to an instance of Employee. And it need not return anything, since you are just filling in a field of that employee:

void calc_Bonus( Employee *e ) {
   if(e->basicsalary<=7000 && e->salespercentage<=10) {
     e->bonus = xxx;
   } else if ( ... ) {
     ...
   }
}

Then just call it using

   calc_Bonus( &emp[i] );

Answer 3

Given emp declared in main() like so:

struct Employee *emp = malloc(n*sizeof(*emp));

, emp is a single pointer (as opposed to an array of pointers) to space large enough for several struct Employee objects; semantically, it points specifically to the first of those objects.

In that case, this call ...

calc_Bonus(&emp,i)

... and the similar call to calc_NetSalary(&emp,i) are consistent with those functions' prototypes, but inconsistent with their implementations and the joint meaning of their argument lists (as implied by their implementations) when i is nonzero. Specifically, consider this expression from calc_Bonus():

emp[i]->basicsalary

Since emp is declared there as a struct Employee **, let's rewrite that to replace the [] operation with the equivalent pointer-arithmetic based expression:

(*(emp + i))->basicsalary

Now the problem should be clearer, but you have to be careful to distinguish between the emp of calc_Bonus() and the emp of main(), which differ in both type and value. The emp in calc_Bonus(), as called, points to a scalar (emp of main()), so if you add a nonzero integer to it and attempt to dereference the result then you get undefined behavior.

Given the current function signature, that expression and all the similar ones would need to be rewritten in this form:

(*emp)[i].basicsalary

... or this one ...

(*emp + i)->basicsalary

But I think a better solution would be to reduce the level of indirection by rewriting the function like so:

int calc_Bonus(struct Employee *emp, int i) {
    if (emp[i].basicsalary <= 7000 && emp[i].salespercentage <= 10)
        return 1500;
    // ...
}

and calling it from main() like so:

calc_Bonus(emp,i);

There are several other valid alternatives that would also be better than the original code.