Program stops working after entering the value for more than 6 users

Question

I have a school project which asks me to write a code using struct and functions, regarding sales in Greek cities. You are prompted to enter the values of 'em. The program works just fine if I enter that there's 6 salesmen, but a number larger than that makes it stop after entering all the values.. An example would be:

Number of salesmen: 7

(

Enter the his id:
Enter his surname:
Enter the number of sales:
Enter the area code:

) x7 times.

Then, when you enter the last value, it would just stop. Not exit, not stop responding/crash, literally just stop, and you would be unable to type anything more, only option would be to exit.

Thing is, there's no error or warning in both the build messages and the log, which has me quite confused.

My guess is that the error is in the function "calcSales" but I wanted to post the whole code just in case you need more info on it.

Could you take a look at the code and tell me if you find anything wrong? Thank you.

#include <stdio.h>
#include "genlib.h"
#include <simpio.h>
#include <string.h>
#define N 20
#define M 4

struct{
     int id;
     char surname[16];
     long sales;
     int area;
} salesmen[N];

void info(int *count);
void calcSales(int *count);

int main(){

     int count;

     printf("Give me the number of salesmen:\n");
     count=GetInteger();

     info(&count);

     calcSales(&count);
}

void info(int *count){

for (int i=0; i<*count; i++){
    printf("\nInfo for salesman number %d:\n", i+1);

    printf("\nGive me his id: ");
        salesmen[i].id=GetInteger();

    printf("\nGive me his surname: ");
        gets(salesmen[i].surname); 

    printf("\nGive me the number of sales: ");
        salesmen[i].sales=GetLong();

    printf("\n 1=Thessaloniki, 2= Athens, 3= Volos, 4= Hrakleio \n");
    printf("\nLastly, give me the number of his area: ");
        salesmen[i].area=GetInteger();

    if (salesmen[i].area>4){
        printf("\nThe number you are trying to enter doesn't match to an area.\n");
        break;
        }
    }
}

void calcSales(int *count){


    long tSales[4];
    for (int i=0; i<*count; i++){
    tSales[i]=0;
    }

for (int i=0; i<*count; i++){

    if(salesmen[i].area==1){
            tSales[0]+=salesmen[i].sales;
        }
    if(salesmen[i].area==2){
            tSales[1]+=salesmen[i].sales;
        }
    if(salesmen[i].area==3){
            tSales[2]+=salesmen[i].sales;
        }
    if(salesmen[i].area==4){
            tSales[3]+=salesmen[i].sales;
    }
}

for (int i=0; i<4; i++){
    printf("\nSales for area number %d: %ld\n",i+1, tSales[i]);
    }
}

Look at how many elements there are in `tSales`, then look at how many elements you're accessing in the loop that zeroes it. — molbdnilo, Dec 29 '19 at 12:55
You have variable called `tSaales` but following line uses `tSales`. Is that a mistake ? Also why pass a pointer count to function ? why not just an integer ? — Youssef13, Dec 29 '19 at 12:57
@Youssef13 ah typing mistake when converting it from greek to english. Because our professor wants to make our lives hard :D — Vaggelis Davios, Dec 29 '19 at 12:59
In the line: `tSales[i] = 0;` . What values `i` is taking ? it's from 0 to 19. But the actual size of that array is 4. You're accessing memory that you don't have. — Youssef13, Dec 29 '19 at 12:59
@molbdnilo AH You're right. I'll try to set it < 4 and see what that does. — Vaggelis Davios, Dec 29 '19 at 13:02
The best way to avoid this problem is to properly initialise the array – `long tSales[4] = {0};` – instead of using the "declare, then assign" method. — molbdnilo, Dec 29 '19 at 13:02
@RahulBharadwaj, Fully agree! It can cause buffer overflow. But it's a quite bit advanced for someone who is just starting learning C. — Youssef13, Dec 29 '19 at 13:03
@Youssef13 Thank you for explaining the issue, I have now fixed it by setting the first for loop to 4. — Vaggelis Davios, Dec 29 '19 at 13:04
I think for beginners, you can do this: `char tmp[16]; scanf("%s", tmp); strncpy(salesmen[i].surname, tmp, 16);` . — Rahul Bharadwaj, Dec 29 '19 at 13:04
@molbdnilo thank you as well for helping me solve this! One more question, what's the difference between long tSales[4] = {0}; and long tSales[4]=0; ? Is it because the tSales is an array and you're basically zero'ing all the elements in? or just zero'ing the first one? — Vaggelis Davios, Dec 29 '19 at 13:06
@RahulBharadwaj We have been taught both strncpy and gets but to me gets really seems much easier! Would you say that the strncpy version can be better and less buggy for a beginner? — Vaggelis Davios, Dec 29 '19 at 13:07
@RahulBharadwaj, scanf has the problem that it stops when it encounter a space. — Youssef13, Dec 29 '19 at 13:08
I would agree, `gets` just seems very easy in beginning, but its good to learn a few gotchas early on. You can read about why `gets` is bad here: https://stackoverflow.com/a/4309845/6400614 . Also, if you ever feel to take a dynamic length string via user input, you can refer this: https://stackoverflow.com/a/7672607/6400614 — Rahul Bharadwaj, Dec 29 '19 at 13:09
@RahulBharadwaj Thank you for providing so much information! I love how you guys are generous enough to share so much although it's a google search away. — Vaggelis Davios, Dec 29 '19 at 13:15

Youssef13 · Accepted Answer · 2019-12-29T13:13:58.410

Your guess is correct. The mistake in calcSales function.

Exactly, the following part:

long tSales[4];
for (int i=0; i<*count; i++) {
        tSales[i]=0;
}

The *count has value of 20. Which means the loop goes from i = 0 until i = 19. When you access tSales[i] for i = 4 and above. You're invoking an undefined behavior. It's a memory that you didn't reserve for tSales. I suggest using the following:

long tSales[4];
for (int i=0; i<4; i++) {
        tSales[i]=0;
}

Or better:

long tSales[4];
for (int i=0; i<sizeof(tSales)/sizeof(tSales[0]); i++) { // number of elements is the total size of array divided by the size of one element.
        tSales[i]=0;
}

Or even better, you don't need a loop at all:

long tSales[4] = {0};

Besides your problem:

For the code you're providing, you don't need to pass a pointer to the functions.

You can make the code look as follows:

#include <stdio.h>
#include "genlib.h"
#include <simpio.h>
#include <string.h>
#define N 20
#define M 4

struct{
     int id;
     char surname[16];
     long sales;
     int area;
} salesmen[N];

void info(int count);
void calcSales(int count);

int main(){

     printf("Give me the number of salesmen:\n");
     int count=GetInteger(); // I've made declaration and assignment in same line. That seems cleaner to me.

     info(count);
     calcSales(count);
}

void info(int count){

    for (int i=0; i<count; i++){
        printf("\nInfo for salesman number %d:\n", i+1);

        printf("\nGive me his id: ");
        salesmen[i].id=GetInteger();

        printf("\nGive me his surname: ");
        gets(salesmen[i].surname); 

        printf("\nGive me the number of sales: ");
        salesmen[i].sales=GetLong();

        printf("\n 1=Thessaloniki, 2= Athens, 3= Volos, 4= Hrakleio \n");
        printf("\nLastly, give me the number of his area: ");
        salesmen[i].area=GetInteger();

        if (salesmen[i].area>4 || salesmen[i].area < 1){ // Added an extra condition.
            printf("\nThe number you are trying to enter doesn't match to an area.\n");
            break;
        }
    }
}

void calcSales(int count){

    long tSales[4] = {0};

    for (int i=0; i<count; i++){
            tSales[salesmen[i].area - 1]+=salesmen[i].sales; // No need for if conditions.
    }

    for (int i=0; i<4; i++){
        printf("\nSales for area number %d: %ld\n",i+1, tSales[i]);
    }
}

Hi Youssef13, could you show OP a small replacement code for the `gets` part (without using `scanf`). — Rahul Bharadwaj, Dec 29 '19 at 13:10
Thank you so much for helping me solve it but also for showing all the possible ways. Cheers!! — Vaggelis Davios, Dec 29 '19 at 13:13
@RahulBharadwaj, Personally, I prefer using fgets. Both scanf and gets aren't safe. fgets is safe. Reasoning for that can be explained separately. But all the thing is about buffer overflow — Youssef13, Dec 29 '19 at 13:15

Program stops working after entering the value for more than 6 users

1 Answers1