Skip to content

commit 382bff02 claims to fix use-after-free in udp_cleanup() but doesn't (and doesn't make coverity happy anyway)

Commit 382bff02 in slirp claims to be fixing a use-after-free in udp_cleanup() and icmp_cleanup().

The old code was:

       55|       while (slirp->udb.so_next != &slirp->udb) {
       56|->         udp_detach(slirp->udb.so_next);
       57|       }

Some static analysis tool (Coverity?) presumably thinks this is a use-after-free because it correctly identifies that udp_detach() frees the struct socket passed to it. However the analysis tool has not identified that it also removes the struct socket from the list that it is on. (It calls sofree(), which calls remque(), which does the updating of the so_next/so_prev links. The tool was probably confused by the way that remque() works with a struct quehead and implicitly requires that struct socket starts with so_next/so_prev pointers in the same layout as the struct quehead).

So the old code was fine and didn't have a bug.

However the new code in commit 382bff02 still doesn't placate Coverity: the new code is

    for (so = slirp->udb.so_next; so != &slirp->udb; so = so_next) {
         so_next = so->so_next;
         udp_detach(slirp->udb.so_next);
     }

The change to the argument to udp_detach() has been forgotten (compare the change in the same commit to icmp_cleanup()), so Coverity still thinks that we're using slirp->udb.so_next after it is freed. (We aren't, for the same reason that the original code was fine.)

I think we could either just revert 382bff02 and dismiss the Coverity issue, or change the argument to udp_detach() to 'so' so it's consistent with the icmp_cleanup() code. Personally I would revert 382bff02 -- it makes the code look like it's doing the C idiom of "iterate down a list freeing all the items, prior to freeing or otherwise invalidating the object that held the list head", but in fact the list remains in a valid state the whole time.

(The pointer games remque() plays are rather dubious and I'm not surprised that the analysis tools are confused by them (this code might even be undefined-behaviour, I'm not sure). In an ideal world it would be better to replace this with a linked-list system which didn't lie to the compiler about what the type of the struct it's been passed is, such as QEMU's queue.h macros. That would likely be a lot of work, though.

(QEMU's Coverity-Scan system flagged this up as CID 1432309.)

Edited by Peter Maydell