Patrick J. LoPresti
2001-Jun-04 15:01 UTC
OpenSSH 2.9p1: lastlog_get_entry() should be more careful
In OpenSSH 2.9p1, the function loginrec.c:lastlog_get_entry() looks like this: int lastlog_get_entry(struct logininfo *li) { struct lastlog last; int fd; if (lastlog_openseek(li, &fd, O_RDONLY)) { if (atomicio(read, fd, &last, sizeof(last)) != sizeof(last)) { log("lastlog_get_entry: Error reading from %s: %s", LASTLOG_FILE, strerror(errno)); return 0; } else { lastlog_populate_entry(li, &last); return 1; } } else { return 0; } } On Linux (at least version 2.2.19), the call to lastlog_openseek() can succeed even when the /var/log/lastlog file is short (e.g., 0 bytes). Subsequent calls to read() on the file descriptor return zero (EOF). Consequently, the call to atomicio in the function above returns zero, causing OpenSSH to log a bogus error message by examining a bogus errno value. I suggest that the call to atomicio() be checked to see if it returns zero, with no error message generated for this case. That is, I suggest the function be rewritten something like this: int lastlog_get_entry(struct logininfo *li) { struct lastlog last; int fd; if (lastlog_openseek(li, &fd, O_RDONLY)) { int ret = atomicio(read, fd, &last, sizeof(last)); if (ret == sizeof(last)) { lastlog_populate_entry(li, &last); return 1; } else if (ret == 0) { return 0; } else { log("lastlog_get_entry: Error reading from %s: %s", LASTLOG_FILE, strerror(errno)); return 0; } } else { return 0; } } The above is just for illustration; it is still not quite correct. Strictly speaking, you should not examine errno unless atomicio() returns a negative value, because a short read will not set errno either. - Pat