Skip to content

Fix fetch context use-after-free bugs

fctx_decreference() may call fctx_destroy(), which in turn may free the fetch context by calling isc_mem_putanddetach(). This means that whenever fctx_decreference() is called, the fetch context pointer should be assumed to point to garbage after that call. Meanwhile, the following pattern is used in several places in lib/dns/resolver.c:

LOCK(&res->buckets[fctx->bucketnum].lock);
bucket_empty = fctx_decreference(fctx);
UNLOCK(&res->buckets[fctx->bucketnum].lock);

Given that 'fctx' may be freed by the fctx_decreference() call, there is no guarantee that the value of fctx->bucketnum will be the same before and after the fctx_decreference() call. This can cause all kinds of locking issues as LOCK() calls no longer match up with their UNLOCK() counterparts.

Fix by always using a helper variable to hold the bucket number when the pattern above is used.

Note that fctx_try() still uses 'fctx' after calling fctx_decreference() (it calls fctx_done()). This is safe to do because the reference count for 'fctx' is increased a few lines earlier and it also cannot be zero right before that increase happens, so the fctx_decreference() call in that particular location never invokes fctx_destroy(). Nevertheless, use a helper variable for that call site as well, to retain consistency and to prevent copy-pasted code from causing similar problems in the future.

Pipeline including a full matrix of "stress" tests: https://gitlab.isc.org/isc-projects/bind9/-/pipelines/109460

The code fixed by this MR was reworked in BIND 9.17.20 by !5500 (merged), so newer versions are not affected.

Closes #3441 (closed)

Edited by Michał Kępień

Merge request reports