g10/keydb.c uses a keyblock cache. This is currently a global cache. The
functions, however, assume that the cache is owned by a database handle. Thus,
if we do the following:
keydb_search_fpr (hd1, fpr1); keydb_search_fpr (hd2, fpr2); keydb_get_keyblock (hd2, ...); keydb_get_keyblock (hd1, ...);
the second call to keydb_get_keyblock will (should) return the keyblock
associated with fpr2, not fpr1!
Consider the start of keydb_get_keyblock:
gpg_error_t keydb_get_keyblock (KEYDB_HANDLE hd, KBNODE *ret_kb) { ... if (hd->keyblock_cache.state == KEYBLOCK_CACHE_FILLED) { iobuf_seek (hd->keyblock_cache.iobuf, 0); err = parse_keyblock_image (hd->keyblock_cache.iobuf, hd->keyblock_cache.pk_no, hd->keyblock_cache.uid_no, hd->keyblock_cache.sigstatus, ret_kb); if (err) keyblock_cache_clear (hd); if (DBG_CLOCK) log_clock (err? "keydb_get_keyblock leave (cached, failed)" : "keydb_get_keyblock leave (cached)"); return err; }
We don't check that the cache entry corresponds with the selected result.
Similarly, when we fill the cache further down in the same function, we don't
check that the prepared area is actually for this keyblock; the area could have
been prepared by another search on a different handle:
err = keybox_get_keyblock (hd->active[hd->found].u.kb, &iobuf, &pk_no, &uid_no, &sigstatus); if (!err) { err = parse_keyblock_image (iobuf, pk_no, uid_no, sigstatus, ret_kb); if (!err && hd->keyblock_cache.state == KEYBLOCK_CACHE_PREPARED) {
Unfortunately, it is not enough to add a check that the cached data is what we
are looking for. It is possible that the same key is in multiple keyrings, but
with different contents (e.g., one has a revocation certificate and the other
doesn't).
Another interesting effect is that if keydb_search fails and the caller proceeds
to call keydb_getblock anyways data will be returned instead of an error message
if the cache is filled!
The attached patch makes the cache a per-handle cache. There is currently one
gotcha: in keydb_rebuild_caches, we invalidate the keyblock cache (but,
interestingly no the not found cache). Since we don't have a list of all
handles, it's not clear how to invalidate the cache. But, is it really
necessary? Reindexing shouldn't change the contents only the layout on disk,
right? (I'm sorry for my ignorance, but I haven't taken the time to figure out
exactly what this function does.) Further, the cache is only used for keyboxes
and keydb_rebuild_caches only works on keyrings (it's a noop for keyboxes). So,
I think it is safe to not invalidate the cache in this case.