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?

4
  • 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. Mar 18, 2013 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? Mar 18, 2013 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. Mar 18, 2013 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, 2013 at 19:56

2 Answers 2

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.

2
  • 1
    (said with a smile) Didn't I just say that in my earlier answer? Mar 18, 2013 at 19:56
  • 4
    @SteveValliere More or less, but I tried to be more explicit about the underlying concepts. Mar 18, 2013 at 19:58
3

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.

7
  • 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). Mar 18, 2013 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. Mar 18, 2013 at 20:04
  • 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. Oct 12, 2017 at 19:58
  • 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, 2021 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, 2021 at 17:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.