From: Petr Špaček <petr.spacek@nic.cz>
Date: Thu, 7 Dec 2017 16:45:58 +0000 (+0100)
Subject: rplan: fix kr_qflags_*() to work with more than 32 flags
X-Git-Tag: v1.5.1~5^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4f79b91d50a7da95da8d0b48000fa8b5b35ec26f;p=thirdparty%2Fknot-resolver.git

rplan: fix kr_qflags_*() to work with more than 32 flags

Originally division around sizeof(uint32_t) caused silent truncation
for struct kr_qflags with sizes not multiple of 4 bytes.

Attempts to optimize using uint32_t blocks could lead to read/write
beyond end of uint32_t so I'm not willing to risk it.

Also, the code was refactored to avoid duplication between _set and _clear.
Quick look into assembly produced by gcc 7.2.1 with -O2 on x86_64 confirms that
all auxiliary functions got inlined so there are not extra function calls.

Unit tests are attached. These fail on the previous version of _set() and
_clear() and work now.
---

diff --git a/lib/rplan.c b/lib/rplan.c
index 6bdd87c9d..c4a327992 100644
--- a/lib/rplan.c
+++ b/lib/rplan.c
@@ -27,37 +27,58 @@
 #define QUERY_PROVIDES(q, name, cls, type) \
     ((q)->sclass == (cls) && (q)->stype == type && knot_dname_is_equal((q)->sname, name))
 
-void kr_qflags_set(struct kr_qflags *fl1, struct kr_qflags fl2)
+inline static unsigned char chars_or(const unsigned char a, const unsigned char b)
+{
+	return a | b;
+}
+
+/** Bits set to 1 in variable b will be set to zero in variable a. */
+inline static unsigned char chars_mask(const unsigned char a, const unsigned char b)
+{
+	return a & ~b;
+}
+
+/** Apply mod(a, b) to every byte a, b from fl1, fl2 and return result in fl1. */
+inline static void kr_qflags_mod(struct kr_qflags *fl1, struct kr_qflags fl2,
+			unsigned char mod(const unsigned char a, const unsigned char b))
 {
 	if (!fl1) abort();
-	typedef uint32_t ui;
 	union {
 		struct kr_qflags flags;
-		ui uints[sizeof(struct kr_qflags) / sizeof(ui)];
+		/* C99 section 6.5.3.4: sizeof(char) == 1 */
+		unsigned char chars[sizeof(struct kr_qflags)];
 	} tmp1, tmp2;
 	/* The compiler should be able to optimize all this into simple ORs. */
 	tmp1.flags = *fl1;
 	tmp2.flags = fl2;
-	for (size_t i = 0; i < sizeof(tmp1.uints) / sizeof(tmp1.uints[0]); ++i) {
-		tmp1.uints[i] |= tmp2.uints[i];
+	for (size_t i = 0; i < sizeof(struct kr_qflags); ++i) {
+		tmp1.chars[i] = mod(tmp1.chars[i], tmp2.chars[i]);
 	}
 	*fl1 = tmp1.flags;
 }
+
+/**
+ * Set bits from variable fl2 in variable fl1.
+ * Bits which are not set in fl2 are not modified in fl1.
+ *
+ * @param[in,out] fl1
+ * @param[in] fl2
+ */
+void kr_qflags_set(struct kr_qflags *fl1, struct kr_qflags fl2)
+{
+	kr_qflags_mod(fl1, fl2, chars_or);
+}
+
+/**
+ * Clear bits from variable fl2 in variable fl1.
+ * Bits which are not set in fl2 are not modified in fl1.
+ *
+ * @param[in,out] fl1
+ * @param[in] fl2
+ */
 void kr_qflags_clear(struct kr_qflags *fl1, struct kr_qflags fl2)
 {
-	if (!fl1) abort();
-	typedef uint32_t ui;
-	union {
-		struct kr_qflags flags;
-		ui uints[sizeof(struct kr_qflags) / sizeof(ui)];
-	} tmp1, tmp2;
-	/* The compiler should be able to optimize all this into simple ORs. */
-	tmp1.flags = *fl1;
-	tmp2.flags = fl2;
-	for (size_t i = 0; i < sizeof(tmp1.uints) / sizeof(tmp1.uints[0]); ++i) {
-		tmp1.uints[i] &= ~tmp2.uints[i];
-	}
-	*fl1 = tmp1.flags;
+	kr_qflags_mod(fl1, fl2, chars_mask);
 }
 
 static struct kr_query *query_create(knot_mm_t *pool, const knot_dname_t *name, uint32_t uid)
diff --git a/tests/test_rplan.c b/tests/test_rplan.c
index 08c6611aa..ad083fd48 100644
--- a/tests/test_rplan.c
+++ b/tests/test_rplan.c
@@ -56,11 +56,31 @@ static void test_rplan_push(void **state)
 	kr_rplan_deinit(&rplan);
 }
 
+/**
+ * Set and clear must not omit any bit, especially around byte boundaries.
+ */
+static void test_rplan_flags(void **state)
+{
+	static struct kr_qflags f1, f2, ones, zeros; /* static => initialized to zeroes */
+	assert_true(memcmp(&f1, &f2, sizeof(f1)) == 0); /* sanity check */
+	memset(&ones, 0xff, sizeof(ones)); /* all ones */
+
+	/* test set */
+	kr_qflags_set(&f1, ones);
+	assert_true(memcmp(&f1, &ones, sizeof(f1)) == 0);  /* 1 == 1 */
+
+	/* test clear */
+	memset(&f2, 0xff, sizeof(f2)); /* all ones */
+	kr_qflags_clear(&f2, ones);
+	assert_true(memcmp(&f2, &zeros, sizeof(f1)) == 0);  /* 0 == 0 */
+}
+
 int main(void)
 {
 	const UnitTest tests[] = {
 	        unit_test(test_rplan_params),
-	        unit_test(test_rplan_push)
+	        unit_test(test_rplan_push),
+	        unit_test(test_rplan_flags)
 	};
 
 	return run_tests(tests);