12

I'm writing a function that perform some authentications actions. I have a file with all the user_id:password:flag couples structured like this:

Users.txt

user_123:a1b2:0 user_124:a2b1:1 user_125:a2b2:2

This is the code:

int main(){
    /*...*/

    /*user_id, password retrieving*/
    USRPSW* p = malloc(sizeof(USRPSW));
    if(p == NULL){
        fprintf(stderr, "Dynamic alloc error\n");
        exit(EXIT_FAILURE);
    }
    memset((void*)p, 0, sizeof(USRPSW));

    if(usr_psw_read(acc_sock_ds, p->user_id, USR_SIZE) <= 0){
        printf("Failed read: connection with %s aborted.\n",
                 inet_ntoa(client_addr.sin_addr));
        close(acc_sock_ds);
        continue;
    }

    if(usr_psw_read(acc_sock_ds, p->password, PSW_SIZE) <= 0){
        printf("Failed read: connection with %s aborted.\n",
                 inet_ntoa(client_addr.sin_addr));
        close(acc_sock_ds);
        continue;
    }

    /*Authentication through user_id, password*/
    FILE *fd;
    fd = fopen(USERSFILE, "r");
    if(fd == NULL){
        fprintf(stderr, "Users file opening error\n");
        exit(EXIT_FAILURE);
    }

    char *usr_psw_line = malloc(USR_SIZE+PSW_SIZE+3+1);
    if(usr_psw_line == NULL){
        fprintf(stderr, "Dynamic alloc error\n");
        exit(EXIT_FAILURE);
    }

    while(1){

        memset((void*)usr_psw_line, 0, sizeof(USR_SIZE+PSW_SIZE+3+1));
        fgets(usr_psw_line, USR_SIZE+PSW_SIZE+3+1, fd);
        printf("%s\n", usr_psw_line);
        fseek(fd, 1, SEEK_CUR);


        /*EOF management*/
        /*usr_id - password matching checking */

    }   
/*...*/    
}

How can I manage the EOF reaching? I saw that when EOF is reached fgets doesn't edits anymore the usr_psw_line but neither returns a NULL pointer. If EOF is reached it mean that no match are found in the users file and the loop breaks.

Can someone give me some tips or suggests?

Fabio Carello
  • 1,062
  • 3
  • 12
  • 30
  • If the end of file is reached without reading any characters, `fgets` is obliged to return `NULL`. Anyway, you could check `feof(fd)` after `fgets` didn't read anything. – Daniel Fischer Mar 18 '13 at 18:29
  • I'm afraid that EOF is not set. I only write a file with records inside.I should also explicitly set the EOF? How do you do this? – Fabio Carello Mar 18 '13 at 18:34
  • Isn't it about reaching the end of file while reading? You don't set EOF, if the end of the file is reached, the next attempt to read from it will set the flag in `*fd`, so `feof(fd)` will then return true if everything works as it should. If everything is not working as it should, hmmm. – Daniel Fischer Mar 18 '13 at 18:41
  • 1
    `EOF` is a condition, not part of the stream. Imagine the stream is water from a tap. When you open the tap until it exhausts do you get **more** water (in another color?) to signal there is no more water? – pmg Mar 18 '13 at 19:56

2 Answers2

29

fgets() return a null pointer when it reaches end-of-file or an error condition.

(EOF is a macro that specifies the value returned by certain other functions in similar conditions; it's not just an abbreviation for the phrase "end of file".)

You're ignoring the result returned by fgets(). Don't do that.

Note that just checking feof(fd) won't do what you want. feof() returns a true result if you've reached the end of the file. If you encounter an error instead, feof() still returns false, and you've got yourself an infinite loop if you're using feof() to decide when you're done. And it doesn't return true until after you've failed to read input.

Most C input functions return some special value to indicate that there's nothing more to read. For fgets() it's NULL, for fgetc() it's EOF, and so forth. If you like, you can call feof() and/or ferror() afterwards to determine why there's nothing more to read.

Keith Thompson
  • 254,901
  • 44
  • 429
  • 631
2

You might want to try something like this in your loop:

while(1)
{
    memset((void*)usr_psw_line, 0, sizeof(USR_SIZE+PSW_SIZE+3+1));
    if( !fgets(usr_psw_line, USR_SIZE+PSW_SIZE+3+1, fd)
     || ferror( fd ) || feof( fd ) )
    {
        break;
    }
    printf("%s\n", usr_psw_line);
    fseek(fd, 1, SEEK_CUR);

    /*EOF management*/
    /*usr_id - password matching checking */

}

With the extra code, the loop will terminate if fgets returns NULL (no more data to read) or if you're read the EOF mark or had any error on the file. I'm sure it is overkill, but those tests have always worked for me.

Steve Valliere
  • 1,119
  • 7
  • 12
  • 2
    It would be best (IMO) to use `while (fgets(usr_psw_line, sizeof(usr_psw_line), fd) != 0)` for the loop condition. There's really not much need to zero the memory before the read. And there's no benefit to testing with `ferror()` or `feof()` there (since they'd only evaluate to true if the test on the return from `fgets()` returns NULL; therefore, they never cause the loop to exit). – Jonathan Leffler Mar 18 '13 at 20:02
  • Agreed. I was trying to leave as much original code intact as I could so that just the parts that directly answered the question were clear. But I do exactly what you describe in all of my file reading loops. – Steve Valliere Mar 18 '13 at 20:04
  • @JonathanLeffler "testing with ferror() ... only evaluate to true if the test on the return from fgets() returns NULL" --> I do not think so in a corner case. `ferror()` returns the status of the error flag which may have been set prior to the `fgets()` call. `fgets()` does not clear that flag so `fgets()` may return non-`NULL` yet `ferror()` returns true. – chux - Reinstate Monica Oct 12 '17 at 19:40
  • 2
    @chux: My argument is based on the proposition that if the error flag is set when `fgets()` is called, `fgets()` must fail. However, I can't actually find verbiage in the standard that mandates that, though I'd be astonished to find an implementation that didn't behave like that. If you track the state of stream, you won't be using it after a failure unless you use `clearerr()`. There is an implausible edge case available. Testing `feof()` is pointless. I stand by my observation for everyday use. If I was worried about the error state of the stream, I'd use `ferror()` on entry to the function. – Jonathan Leffler Oct 12 '17 at 19:58
  • @JonathanLeffler Agree about everyday use, `feof()`, obscureness and good code would test `ferror()`, yet it was the [_only_](https://stackoverflow.com/questions/15484106/reaching-eof-with-fgets/15485047?noredirect=1#comment21920828_15485047) that caught my eye. `fgets()` does not automatically return `NULL` if the stream's _error indicator_ is set, but will return `NULL` is a read error occurs (which has the side effect of setting the _error indicator_). – chux - Reinstate Monica Oct 12 '17 at 21:05
  • 1
    The code "sizeof(USR_SIZE+PSW_SIZE+3+1)" in the example is always computing the size of an integer - on many modern machines it's always going to be 4, regardless of the values of USR_SIZE and PSW_SIZE -- "sizeof(42)" (or any other smallish number) would return the same value. I'm guessing the enclosing "sizeof(...)" is a mistake. Zeroing the entire buffer is overkill anyway - if one were suspicious of the behavior of fgets() - a C string based function - then "usr_psw_line[0] = '\0';" would be sufficient. – CarlRJ Jul 28 '21 at 17:17
  • 1
    And if feof() and/or ferror() are going to be used, they should probably be called either before the call to fgets() - in order to determine if the given file is _already_ in an EOF or error state from prior activities - or _after_ fgets() returns NULL, to determine why fgets() has returned NULL. In the current form, they do little other than to misdirect someone looking at the code. – CarlRJ Jul 28 '21 at 17:18