Comment 17 for bug 997217

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Ok, trusty isn't affected because the loop there has an exit clause:
+ ret = read(s, rbuf+rc, sizeof(rbuf)-rc);
+ if ( ret<0 ) {
+ rc = ret;
+ break;
+ } else {
+ if (ret == 0) {
+ loopc += 1;
+ } else {
+ loopc = 0;
+ }
+ if (loopc > sizeof(rbuf)) { // arbitrary chosen value
+ break;
+ }

That comes from trusty's patch named 0034-fix_dovecot_authentication.patch. So trusty does loop for a bit (sizeof(rbuf) is 1000), but won't get stuck. Someone added the loop counter as a safety net, but didn't change the "ret<0" check into "ret<=0" which would also have fixed this.

In precise, that same patch (0034) adds the loop, but *without* an exit clause, hence this bug.

Xenial is interesting. Upstream at some point adopted the patch that does *NOT* exit the loop, but the code is so similar that someone decided the patch from the package was already applied and dropped it from the package, reintroducing the bug.

To add to the confusion, in xenial that patch file was super slightly renamed from 0034_fix_dovecot_authentication.patch to 0034-fix_dovecot_authentication.patch (can you spot what changed?) and got totally different contents:
--- cyrus-sasl2.orig/lib/checkpw.c
+++ cyrus-sasl2/lib/checkpw.c
@@ -587,16 +587,14 @@ static int read_wait(int fd, unsigned de
        /* Timeout. */
        errno = ETIMEDOUT;
        return -1;
- case +1:
- if (FD_ISSET(fd, &rfds)) {
- /* Success, file descriptor is readable. */
- return 0;
- }
- return -1;
    case -1:
        if (errno == EINTR || errno == EAGAIN)
        continue;
    default:
+ if (FD_ISSET(fd, &rfds)) {
+ /* Success, file descriptor is readable. */
+ return 0;
+ }
        /* Error catch-all. */
        return -1;
    }

From bionic onwards, the upstream version has the loop with no loop counter, but it checks read()'s result for <= 0, not just 0, so it's fixed there.

Bottom line, only xenial is currently affected (and precise, but precise is EOL).