]> git.ipfire.org Git - thirdparty/freeradius-server.git/commitdiff
Annotate overflow issues in lo_read() (CID #1604601)
authorJames Jones <jejones3141@gmail.com>
Tue, 13 Aug 2024 21:11:41 +0000 (16:11 -0500)
committerArran Cudbard-Bell <a.cudbardb@freeradius.org>
Tue, 10 Sep 2024 18:35:12 +0000 (12:35 -0600)
Coverity sets itself up in a vicious cycle:
1. It considers the loop check expression to be tainted because
   total is tainted, so outlen is tainted, too.
2. Because of that, outlen - total (passed to read()) is deemed
   overflowed, so the return value r is considered overflowed.
3. Returning total, which is considered overflowed, is another issue.
4. r, which is considered overflowed, is added to r--which is why
   total considered to have overflowed and hence be tainted.

Once we changed the code to not add r to total in the EINTR case,
one can, but Coverity cannot, infer that total will only take on
values in {0,1,...,outlen}, and since both have the same type, total
can represent all such values. read(), as a standard function, is
one it should have a model for, but it doesn't seem to include the
property that the returned value is less than or equal to the passed
number of bytes to read(), and it doesn't have a way to let us
represent it in a custom model.

src/listen/control/conduit.c

index f23ef7c96b9a592b156830fd5b241e1fba8240ec..c53a909e894457a53ca9bbc1d177f4e996a82d04 100644 (file)
@@ -39,8 +39,10 @@ static ssize_t lo_read(int fd, void *out, size_t outlen)
        uint8_t *p = out;
 
        for (total = 0; total < outlen; ) {
+               /* coverity[overflow_sink] */
                r = read(fd, p + total, outlen - total);
 
+               /* coverity[return_overflow] */
                if (r == 0) return total;
 
                if (r < 0) {