BUG/MEDIUM: ebtree: use a byte-per-byte memcmp() to compare memory blocks

As reported in issue #689, there is a subtle bug in the ebtree code used
to compared memory blocks. It stems from the platform-dependent memcmp()
implementation. Original implementations used to perform a byte-per-byte
comparison and to stop at the first non-matching byte, as in this old
example:

   https://www.retro11.de/ouxr/211bsd/usr/src/lib/libc/compat-sys5/memcmp.c.html

The ebtree code has been relying on this to detect the first non-matching
byte when comparing keys. This is made so that a zero-terminated string
can fail to match against a longer string.

Over time, especially with large busses and SIMD instruction sets,
multi-byte comparisons have appeared, making the processor fetch bytes
past the first different byte, which could possibly be a trailing zero.
This means that it's possible to read past the allocated area for a
string if it was allocated by strdup().

This is not correct and definitely confuses address sanitizers. In real
life the problem doesn't have visible consequences. Indeed, multi-byte
comparisons are implemented so that aligned words are loaded (e.g. 512
bits at once to process a cache line at a time). So there is no way such
a multi-byte access will cross a page boundary and end up reading from
an unallocated zone. This is why it was never noticed before.

This patch addresses this by implementing a one-byte-at-a-time memcmp()
variant for ebtree, called eb_memcmp(). It's optimized for both small and
long strings and guarantees to stop after the first non-matching byte. It
only needs 5 instructions in the loop and was measured to be 3.2 times
faster than the glibc's AVX2-optimized memcmp() on short strings (1 to
257 bytes), since that latter one comes with a significant setup cost.
The break-even seems to be at 512 bytes where both version perform
equally, which is way longer than what's used in general here.

This fix should be backported to stable versions and reintegrated into
the ebtree code.

diff --git a/include/import/ebimtree.h b/include/import/ebimtree.h
index 8b12a548d43ca55f9dc5cc0bee2e36dab086f21e..0afbdd13fcfce0bed18732ede42ae8a14504700c 100644 (file)
--- a/include/import/ebimtree.h
+++ b/include/import/ebimtree.h
@@ -62,7 +62,7 @@ __ebim_lookup(struct eb_root *root, const void *x, unsigned int len)
                if (eb_gettag(troot) == EB_LEAF) {
                        node = container_of(eb_untag(troot, EB_LEAF),
                                            struct ebpt_node, node.branches);
-                       if (memcmp(node->key + pos, x, len) != 0)
+                       if (eb_memcmp(node->key + pos, x, len) != 0)
                                goto ret_null;
                        else
                                goto ret_node;
@@ -76,7 +76,7 @@ __ebim_lookup(struct eb_root *root, const void *x, unsigned int len)
                         * value, and we walk down left, or it's a different
                         * one and we don't have our key.
                         */
-                       if (memcmp(node->key + pos, x, len) != 0)
+                       if (eb_memcmp(node->key + pos, x, len) != 0)
                                goto ret_null;
                        else
                                goto walk_left;

diff --git a/include/import/ebmbtree.h b/include/import/ebmbtree.h
index 8262ec61454defc7dc19989ab690e414b21e0468..a01062494a1e4dae9818f86f9de015b7499f47e2 100644 (file)
--- a/include/import/ebmbtree.h
+++ b/include/import/ebmbtree.h
@@ -154,7 +154,7 @@ static forceinline struct ebmb_node *__ebmb_lookup(struct eb_root *root, const v
                if (eb_gettag(troot) == EB_LEAF) {
                        node = container_of(eb_untag(troot, EB_LEAF),
                                            struct ebmb_node, node.branches);
-                       if (memcmp(node->key + pos, x, len) != 0)
+                       if (eb_memcmp(node->key + pos, x, len) != 0)
                                goto ret_null;
                        else
                                goto ret_node;
@@ -168,7 +168,7 @@ static forceinline struct ebmb_node *__ebmb_lookup(struct eb_root *root, const v
                         * value, and we walk down left, or it's a different
                         * one and we don't have our key.
                         */
-                       if (memcmp(node->key + pos, x, len) != 0)
+                       if (eb_memcmp(node->key + pos, x, len) != 0)
                                goto ret_null;
                        else
                                goto walk_left;

diff --git a/include/import/ebtree.h b/include/import/ebtree.h
index 73ca6d5fb278f1257c1a206497150294aa089d33..85ea31d9bf759e303cacc5a7bcd1edf3d6a28253 100644 (file)
--- a/include/import/ebtree.h
+++ b/include/import/ebtree.h
@@ -908,6 +908,7 @@ static forceinline int get_bit(const unsigned char *a, unsigned int pos)
 /* These functions are declared in ebtree.c */
 void eb_delete(struct eb_node *node);
 struct eb_node *eb_insert_dup(struct eb_node *sub, struct eb_node *new);
+int eb_memcmp(const void *m1, const void *m2, size_t len);
 
 #endif /* _EB_TREE_H */
 

diff --git a/src/ebtree.c b/src/ebtree.c
index 3b904209d4072de6c09c15a68d9d7dc936cf87d9..db27875e10dc8d252f4bda4f8468d5786aa33e93 100644 (file)
--- a/src/ebtree.c
+++ b/src/ebtree.c
@@ -30,3 +30,21 @@ struct eb_node *eb_insert_dup(struct eb_node *sub, struct eb_node *new)
 {
        return __eb_insert_dup(sub, new);
 }
+
+/* compares memory blocks m1 and m2 for up to <len> bytes. Immediately stops at
+ * the first non-matching byte. It returns 0 on full match, non-zero otherwise.
+ * One byte will always be checked so this must not be called with len==0. It
+ * takes 2+5cy/B on x86_64 and is ~29 bytes long.
+ */
+int eb_memcmp(const void *m1, const void *m2, size_t len)
+{
+       const char *p1 = (const char *)m1 + len;
+       const char *p2 = (const char *)m2 + len;
+       ssize_t ofs = -len;
+       char diff;
+
+       do {
+               diff = p1[ofs] - p2[ofs];
+       } while (!diff && ++ofs);
+       return diff;
+}