Why does it skip fgets()?

Question

typedef struct {
int serial_no;
char s_name[100];
char s_street[50];
char s_town[20];
int s_speaker_no;
int max_no_teachers;
int no_applied_teachers;
} type_seminar;


void add_seminar() {
int seminar_number, temp_int;
char *temp_string;
temp_string = (char*) malloc(100 * sizeof(char));

FILE *f_sem;
FILE *f_school;
f_sem = fopen("seminars.bin", "a");
f_school = fopen("school_list.txt", "r");

fgets(temp_string, 10, f_school);
seminar_number = (atoi(temp_string) + 1);

type_seminar *temp_s = (type_seminar*) malloc(sizeof(type_seminar));
temp_s->serial_no = seminar_number;
temp_s->no_applied_teachers = 0;
temp_s->s_speaker_no = 0;

printf("Enter the seminar title: \n");
fgets(temp_string, sizeof temp_string, stdin);
strcpy(temp_s->s_name, temp_string);

printf("Enter the seminar address(street and house number): \n");
fgets(temp_string, sizeof temp_string, stdin);
strcpy(temp_s->s_street, temp_string);

printf("Enter the town (where the seminar will be held) : \n");
fgets(temp_string, sizeof temp_string, stdin);
strcpy(temp_s->s_town, temp_string);

printf("Enter the maximum number of the seminar participants : \n");
fgets(temp_string, sizeof temp_string, stdin);
temp_int = (atoi(temp_string));
temp_s->max_no_teachers = temp_int;

free(temp_s);
free(temp_string);
fclose(f_school);
fclose(f_sem);
}

The first fgets() where user should enter the seminar title gets skipped everytime I run the function. I presume the previus fgets() that reads from the txt file leaves something in the buffer? I have no idea how to fix this though... Also, I'm a newbie at C and programming in general, so if it's something obvius... sorry :/

`temp_string` is a pointer. `sizeof temp_string` will not get you the length like you're using it... — Jeff Mercado, Feb 05 '13 at 02:05
I changed it to 100*sizeof(char), which is the actuall size of the pointer if understand malloc function properly... it still doesnt work... — Sara, Feb 05 '13 at 02:14
@user2041470: then you either didn't change it everywhere or you have a _different_ problem :-) — paxdiablo, Feb 05 '13 at 02:18
Why not avoid the dynamic memory allocation; go with `char temp_string[4096];` unless you're on micro-microprocessor system with less than a megabyte of memory. — Jonathan Leffler, Feb 05 '13 at 02:23

score 3 · Answer 1 · edited Jul 02 '18 at 01:29

3

Kudos for using fgets to avoid buffer overflows, but you're not quite there:

char *temp_string;
:
temp_string = (char*) malloc(100 * sizeof(char));
:
fgets(temp_string, sizeof temp_string, stdin);

The size of temp_string is the size of a char pointer, not the size of the buffer you've allocated. That means you're most likely just reading four (or possibly eight if you have 64-bit pointers) characters maximum, then the rest are left in the input stream.

You should be using the size of the buffer (100, though it would be better as a defined constant rather than a hard-coded value).

Alternatively, have a look at this getLine routine, which handles a lot of edge cases.

And, just as an aside, you don't need to multiply by sizeof(char) since that's always guaranteed to be 1 by definition - doing the multiply just clogs up your source code.

You also shouldn't cast the return value from malloc since it can hide certain subtle errors. C is quite capable of implicitly converting thevoid *` return value to any other pointer type.

The other thing you should watch out for: even though you're using fgets to protect the temp_string buffer from overflow, no similar protection has been put in place for your strcpy functions.

That means it will allow you to enter an 80-character town name, which will then blow away memory it's not supposed to touch as you strcpy that into the 20-character structure field.

edited Jul 02 '18 at 01:29

melpomene

84,125
8
85
148

answered Feb 05 '13 at 02:09

paxdiablo

854,327
234
1,573
1,953

Thanks, I tried the 100, and constant, but it still doesnt work. And I made sure I changed it everywhere... not "happy" about the _different_ problem you've implied :/ I will try with the getLine routine first thing tommorow :) – Sara Feb 05 '13 at 02:35
Just clarifying. I didn't mean to indicate _different_ as some sort of personal attack (I'm not sure if that's what you meant as the net doesn't carry all those extra communication signals you get from face-to-face contact). I just meant that the problem would be different to the incorrect-size one. Eg, as per my update, maybe your town name is "Bridgewater on Loddon" which would blow out your structure. – paxdiablo Feb 05 '13 at 02:43
Thought about the strcpy problem actually, I planned to check the temp_string length with the strlen function, just wanted to make sure the fgets works before doing that. And I didnt take _different_ as personal attack, not at all, I was unhappy with myself for writing sth else in the code wrong. Sorry for the missunderstanding! – Sara Feb 05 '13 at 02:50
Em, I thought it would be OK to let you know that I solved the problem. There was a scanf 3 switch functions "before" that I didnt consider, I used the fflush(stdin) to clear the buffer and it works now. Thanks again for all the help!! – Sara Feb 05 '13 at 03:25
@user2041470, that would presumably be the _different_ problem I was referring to :-) Cheers. – paxdiablo Feb 05 '13 at 03:27

Why does it skip fgets()?

1 Answers1