From: Petr Špaček <petr.spacek@nic.cz>
Date: Mon, 7 Sep 2020 14:08:05 +0000 (+0200)
Subject: cache: fix race in assert_right_version
X-Git-Tag: v5.1.3~1^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f986f462459d567b65d2d1af91829d4b34b3c8c7;p=thirdparty%2Fknot-resolver.git

cache: fix race in assert_right_version

This change fixes race condition in assert_right_version(). Racy
situation:
- Two instances have the (empty) cache open: New binary and old binary.
- New binary executes count() inside assert_right_version(), which
  internally starts RO transaction. Returned count is 0.
- Old binary does some writes (RW transaction parallel to RO in the first
  process).
- New binary skips cache clear because cache was empty at the time of check.
- Result: The old binary wrote data with an old format into cache which
  was not cleared and silenty changed version number to a new one.

This is not complete fix because we lack mechanism to detect cache format
change at run-time, but at least it removes one nasty corner case and
cost of this change seems to be minimal.
---

diff --git a/lib/cache/api.c b/lib/cache/api.c
index d7794eca9..306aa603d 100644
--- a/lib/cache/api.c
+++ b/lib/cache/api.c
@@ -77,9 +77,10 @@ static int assert_right_version(struct kr_cache *cache)
 		ret = kr_ok();
 	} else {
 		int oldret = ret;
-		/* Version doesn't match. Recreate cache and write version key. */
+		/* Version doesn't match or we were unable to read it, possibly because DB is empty.
+		 * Recreate cache and write version key. */
 		ret = cache_op(cache, count);
-		if (ret != 0) { /* Non-empty cache, purge it. */
+		if (ret != 0) { /* Log for non-empty cache to limit noise on fresh start. */
 			kr_log_info("[cache] incompatible cache database detected, purging\n");
 			if (oldret) {
 				kr_log_verbose("[cache] reading version returned: %d\n", oldret);
@@ -91,9 +92,8 @@ static int assert_right_version(struct kr_cache *cache)
 				kr_log_verbose("[cache] version has bad value: %d instead of %d\n",
 					(int)ver, (int)CACHE_VERSION);
 			}
-			ret = cache_op(cache, clear);
 		}
-		/* Either purged or empty. */
+		ret = cache_op(cache, clear);
 	}
 	/* Rewrite the entry even if it isn't needed.  Because of cache-size-changing
 	 * possibility it's good to always perform some write during opening of cache. */