Skip to content

Commit b55fcf2

Browse files
wtarreauwtarreau
committed
[BUG] ebtree: fix duplicate strings insertion
(update to ebtree 6.0.4) Recent fix fd301cc1370cd4977fe175dfa4544c7dc0e7ce6b was not OK because it was returning one excess byte, causing some duplicates not to be detected. The reason is that we added 8 bits to count the trailing zero but they were implied by the pre-incrementation of the pointer. Fixing this was still not enough, as the problem appeared when string_equal_bits() was applied on two identical strings, and it returned a number of bits covering the trailing zero. Subsequent calls were applied to the first byte after this trailing zero. It was often zero when doing insertion from raw files, explaining why the issue was not discovered earlier. But when the data is from a reused area, duplicate strings are not correctly detected when inserting into the tree. Several solutions were tested, and the only efficient one consists in making string_equal_bits() notify the caller that the end of the string was reached. It now returns zero and the callers just have to ensure that when they get a zero, they stop using that bit until a dup tree or a leaf is encountered. This fix brought the unexpected bonus of simplifying the insertion code a bit and making it slightly faster to process duplicates. The impact for haproxy was that if many similar string patterns were loaded from a file, there was a potential risk that their insertion or matching could have been slower. The bigger impact was with the URL sorting feature of halog, which is not yet merged and is how this bug was discovered. (cherry picked from commit 518d59ec9ba43705f930f9ece3749c450fd005df)
1 parent 6190b7d commit b55fcf2

File tree

3 files changed

+143
-77
lines changed

3 files changed

+143
-77
lines changed

‎ebtree/ebistree.h

+70-37
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,24 @@ static forceinline struct ebpt_node *__ebis_lookup(struct eb_root *root, const v
8080
return node;
8181
}
8282

83-
/* OK, normal data node, let's walk down */
84-
bit = string_equal_bits(x, node->key, bit);
85-
if (bit < node_bit)
86-
return NULL; /* no more common bits */
83+
/* OK, normal data node, let's walk down but don't compare data
84+
* if we already reached the end of the key.
85+
*/
86+
if (likely(bit >= 0)) {
87+
bit = string_equal_bits(x, node->key, bit);
88+
if (likely(bit < node_bit)) {
89+
if (bit >= 0)
90+
return NULL; /* no more common bits */
91+
92+
/* bit < 0 : we reached the end of the key. If we
93+
* are in a tree with unique keys, we can return
94+
* this node. Otherwise we have to walk it down
95+
* and stop comparing bits.
96+
*/
97+
if (eb_gettag(root->b[EB_RGHT]))
98+
return node;
99+
}
100+
}
87101

88102
troot = node->node.branches.b[(((unsigned char*)x)[node_bit >> 3] >>
89103
(~node_bit & 7)) & 1];
@@ -160,32 +174,41 @@ __ebis_insert(struct eb_root *root, struct ebpt_node *new)
160174
*
161175
* The last two cases can easily be partially merged.
162176
*/
163-
bit = string_equal_bits(new->key, old->key, bit);
164-
diff = cmp_bits(new->key, old->key, bit);
177+
if (bit >= 0)
178+
bit = string_equal_bits(new->key, old->key, bit);
179+
180+
if (bit < 0) {
181+
/* key was already there */
165182

166-
if (diff < 0) {
167-
new->node.leaf_p = new_left;
168-
old->node.leaf_p = new_rght;
169-
new->node.branches.b[EB_LEFT] = new_leaf;
170-
new->node.branches.b[EB_RGHT] = old_leaf;
171-
} else {
172183
/* we may refuse to duplicate this key if the tree is
173184
* tagged as containing only unique keys.
174185
*/
175-
if (diff == 0 && eb_gettag(root_right))
186+
if (eb_gettag(root_right))
176187
return old;
177188

178-
/* new->key >= old->key, new goes the right */
189+
/* new arbitrarily goes to the right and tops the dup tree */
179190
old->node.leaf_p = new_left;
180191
new->node.leaf_p = new_rght;
181192
new->node.branches.b[EB_LEFT] = old_leaf;
182193
new->node.branches.b[EB_RGHT] = new_leaf;
194+
new->node.bit = -1;
195+
root->b[side] = eb_dotag(&new->node.branches, EB_NODE);
196+
return new;
197+
}
183198

184-
if (diff == 0) {
185-
new->node.bit = -1;
186-
root->b[side] = eb_dotag(&new->node.branches, EB_NODE);
187-
return new;
188-
}
199+
diff = cmp_bits(new->key, old->key, bit);
200+
if (diff < 0) {
201+
/* new->key < old->key, new takes the left */
202+
new->node.leaf_p = new_left;
203+
old->node.leaf_p = new_rght;
204+
new->node.branches.b[EB_LEFT] = new_leaf;
205+
new->node.branches.b[EB_RGHT] = old_leaf;
206+
} else {
207+
/* new->key > old->key, new takes the right */
208+
old->node.leaf_p = new_left;
209+
new->node.leaf_p = new_rght;
210+
new->node.branches.b[EB_LEFT] = old_leaf;
211+
new->node.branches.b[EB_RGHT] = new_leaf;
189212
}
190213
break;
191214
}
@@ -201,48 +224,57 @@ __ebis_insert(struct eb_root *root, struct ebpt_node *new)
201224
* the current node's because as long as they are identical, we
202225
* know we descend along the correct side.
203226
*/
204-
if (old_node_bit < 0) {
205-
/* we're above a duplicate tree, we must compare till the end */
206-
bit = string_equal_bits(new->key, old->key, bit);
207-
goto dup_tree;
208-
}
209-
else if (bit < old_node_bit) {
227+
if (bit >= 0 && (bit < old_node_bit || old_node_bit < 0))
210228
bit = string_equal_bits(new->key, old->key, bit);
211-
}
212229

213-
if (bit < old_node_bit) { /* we don't have all bits in common */
214-
/* The tree did not contain the key, so we insert <new> before the node
215-
* <old>, and set ->bit to designate the lowest bit position in <new>
216-
* which applies to ->branches.b[].
230+
if (unlikely(bit < 0)) {
231+
/* Perfect match, we must only stop on head of dup tree
232+
* or walk down to a leaf.
233+
*/
234+
if (old_node_bit < 0) {
235+
/* We know here that string_equal_bits matched all
236+
* bits and that we're on top of a dup tree, then
237+
* we can perform the dup insertion and return.
238+
*/
239+
struct eb_node *ret;
240+
ret = eb_insert_dup(&old->node, &new->node);
241+
return container_of(ret, struct ebpt_node, node);
242+
}
243+
/* OK so let's walk down */
244+
}
245+
else if (bit < old_node_bit || old_node_bit < 0) {
246+
/* The tree did not contain the key, or we stopped on top of a dup
247+
* tree, possibly containing the key. In the former case, we insert
248+
* <new> before the node <old>, and set ->bit to designate the lowest
249+
* bit position in <new> which applies to ->branches.b[]. In the later
250+
* case, we add the key to the existing dup tree. Note that we cannot
251+
* enter here if we match an intermediate node's key that is not the
252+
* head of a dup tree.
217253
*/
218254
eb_troot_t *new_left, *new_rght;
219255
eb_troot_t *new_leaf, *old_node;
220-
dup_tree:
256+
221257
new_left = eb_dotag(&new->node.branches, EB_LEFT);
222258
new_rght = eb_dotag(&new->node.branches, EB_RGHT);
223259
new_leaf = eb_dotag(&new->node.branches, EB_LEAF);
224260
old_node = eb_dotag(&old->node.branches, EB_NODE);
225261

226262
new->node.node_p = old->node.node_p;
227263

264+
/* we can never match all bits here */
228265
diff = cmp_bits(new->key, old->key, bit);
229266
if (diff < 0) {
230267
new->node.leaf_p = new_left;
231268
old->node.node_p = new_rght;
232269
new->node.branches.b[EB_LEFT] = new_leaf;
233270
new->node.branches.b[EB_RGHT] = old_node;
234271
}
235-
else if (diff > 0) {
272+
else {
236273
old->node.node_p = new_left;
237274
new->node.leaf_p = new_rght;
238275
new->node.branches.b[EB_LEFT] = old_node;
239276
new->node.branches.b[EB_RGHT] = new_leaf;
240277
}
241-
else {
242-
struct eb_node *ret;
243-
ret = eb_insert_dup(&old->node, &new->node);
244-
return container_of(ret, struct ebpt_node, node);
245-
}
246278
break;
247279
}
248280

@@ -260,6 +292,7 @@ __ebis_insert(struct eb_root *root, struct ebpt_node *new)
260292

261293
/* We need the common higher bits between new->key and old->key.
262294
* This number of bits is already in <bit>.
295+
* NOTE: we can't get here whit bit < 0 since we found a dup !
263296
*/
264297
new->node.bit = bit;
265298
root->b[side] = eb_dotag(&new->node.branches, EB_NODE);

‎ebtree/ebsttree.h

+70-37
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,24 @@ static forceinline struct ebmb_node *__ebst_lookup(struct eb_root *root, const v
7878
return node;
7979
}
8080

81-
/* OK, normal data node, let's walk down */
82-
bit = string_equal_bits(x, node->key, bit);
83-
if (bit < node_bit)
84-
return NULL; /* no more common bits */
81+
/* OK, normal data node, let's walk down but don't compare data
82+
* if we already reached the end of the key.
83+
*/
84+
if (likely(bit >= 0)) {
85+
bit = string_equal_bits(x, node->key, bit);
86+
if (likely(bit < node_bit)) {
87+
if (bit >= 0)
88+
return NULL; /* no more common bits */
89+
90+
/* bit < 0 : we reached the end of the key. If we
91+
* are in a tree with unique keys, we can return
92+
* this node. Otherwise we have to walk it down
93+
* and stop comparing bits.
94+
*/
95+
if (eb_gettag(root->b[EB_RGHT]))
96+
return node;
97+
}
98+
}
8599

86100
troot = node->node.branches.b[(((unsigned char*)x)[node_bit >> 3] >>
87101
(~node_bit & 7)) & 1];
@@ -158,32 +172,41 @@ __ebst_insert(struct eb_root *root, struct ebmb_node *new)
158172
*
159173
* The last two cases can easily be partially merged.
160174
*/
161-
bit = string_equal_bits(new->key, old->key, bit);
162-
diff = cmp_bits(new->key, old->key, bit);
175+
if (bit >= 0)
176+
bit = string_equal_bits(new->key, old->key, bit);
177+
178+
if (bit < 0) {
179+
/* key was already there */
163180

164-
if (diff < 0) {
165-
new->node.leaf_p = new_left;
166-
old->node.leaf_p = new_rght;
167-
new->node.branches.b[EB_LEFT] = new_leaf;
168-
new->node.branches.b[EB_RGHT] = old_leaf;
169-
} else {
170181
/* we may refuse to duplicate this key if the tree is
171182
* tagged as containing only unique keys.
172183
*/
173-
if (diff == 0 && eb_gettag(root_right))
184+
if (eb_gettag(root_right))
174185
return old;
175186

176-
/* new->key >= old->key, new goes the right */
187+
/* new arbitrarily goes to the right and tops the dup tree */
177188
old->node.leaf_p = new_left;
178189
new->node.leaf_p = new_rght;
179190
new->node.branches.b[EB_LEFT] = old_leaf;
180191
new->node.branches.b[EB_RGHT] = new_leaf;
192+
new->node.bit = -1;
193+
root->b[side] = eb_dotag(&new->node.branches, EB_NODE);
194+
return new;
195+
}
181196

182-
if (diff == 0) {
183-
new->node.bit = -1;
184-
root->b[side] = eb_dotag(&new->node.branches, EB_NODE);
185-
return new;
186-
}
197+
diff = cmp_bits(new->key, old->key, bit);
198+
if (diff < 0) {
199+
/* new->key < old->key, new takes the left */
200+
new->node.leaf_p = new_left;
201+
old->node.leaf_p = new_rght;
202+
new->node.branches.b[EB_LEFT] = new_leaf;
203+
new->node.branches.b[EB_RGHT] = old_leaf;
204+
} else {
205+
/* new->key > old->key, new takes the right */
206+
old->node.leaf_p = new_left;
207+
new->node.leaf_p = new_rght;
208+
new->node.branches.b[EB_LEFT] = old_leaf;
209+
new->node.branches.b[EB_RGHT] = new_leaf;
187210
}
188211
break;
189212
}
@@ -199,48 +222,57 @@ __ebst_insert(struct eb_root *root, struct ebmb_node *new)
199222
* the current node's because as long as they are identical, we
200223
* know we descend along the correct side.
201224
*/
202-
if (old_node_bit < 0) {
203-
/* we're above a duplicate tree, we must compare till the end */
204-
bit = string_equal_bits(new->key, old->key, bit);
205-
goto dup_tree;
206-
}
207-
else if (bit < old_node_bit) {
225+
if (bit >= 0 && (bit < old_node_bit || old_node_bit < 0))
208226
bit = string_equal_bits(new->key, old->key, bit);
209-
}
210227

211-
if (bit < old_node_bit) { /* we don't have all bits in common */
212-
/* The tree did not contain the key, so we insert <new> before the node
213-
* <old>, and set ->bit to designate the lowest bit position in <new>
214-
* which applies to ->branches.b[].
228+
if (unlikely(bit < 0)) {
229+
/* Perfect match, we must only stop on head of dup tree
230+
* or walk down to a leaf.
231+
*/
232+
if (old_node_bit < 0) {
233+
/* We know here that string_equal_bits matched all
234+
* bits and that we're on top of a dup tree, then
235+
* we can perform the dup insertion and return.
236+
*/
237+
struct eb_node *ret;
238+
ret = eb_insert_dup(&old->node, &new->node);
239+
return container_of(ret, struct ebmb_node, node);
240+
}
241+
/* OK so let's walk down */
242+
}
243+
else if (bit < old_node_bit || old_node_bit < 0) {
244+
/* The tree did not contain the key, or we stopped on top of a dup
245+
* tree, possibly containing the key. In the former case, we insert
246+
* <new> before the node <old>, and set ->bit to designate the lowest
247+
* bit position in <new> which applies to ->branches.b[]. In the later
248+
* case, we add the key to the existing dup tree. Note that we cannot
249+
* enter here if we match an intermediate node's key that is not the
250+
* head of a dup tree.
215251
*/
216252
eb_troot_t *new_left, *new_rght;
217253
eb_troot_t *new_leaf, *old_node;
218-
dup_tree:
254+
219255
new_left = eb_dotag(&new->node.branches, EB_LEFT);
220256
new_rght = eb_dotag(&new->node.branches, EB_RGHT);
221257
new_leaf = eb_dotag(&new->node.branches, EB_LEAF);
222258
old_node = eb_dotag(&old->node.branches, EB_NODE);
223259

224260
new->node.node_p = old->node.node_p;
225261

262+
/* we can never match all bits here */
226263
diff = cmp_bits(new->key, old->key, bit);
227264
if (diff < 0) {
228265
new->node.leaf_p = new_left;
229266
old->node.node_p = new_rght;
230267
new->node.branches.b[EB_LEFT] = new_leaf;
231268
new->node.branches.b[EB_RGHT] = old_node;
232269
}
233-
else if (diff > 0) {
270+
else {
234271
old->node.node_p = new_left;
235272
new->node.leaf_p = new_rght;
236273
new->node.branches.b[EB_LEFT] = old_node;
237274
new->node.branches.b[EB_RGHT] = new_leaf;
238275
}
239-
else {
240-
struct eb_node *ret;
241-
ret = eb_insert_dup(&old->node, &new->node);
242-
return container_of(ret, struct ebmb_node, node);
243-
}
244276
break;
245277
}
246278

@@ -258,6 +290,7 @@ __ebst_insert(struct eb_root *root, struct ebmb_node *new)
258290

259291
/* We need the common higher bits between new->key and old->key.
260292
* This number of bits is already in <bit>.
293+
* NOTE: we can't get here whit bit < 0 since we found a dup !
261294
*/
262295
new->node.bit = bit;
263296
root->b[side] = eb_dotag(&new->node.branches, EB_NODE);

‎ebtree/ebtree.h

+3-3
Original file line numberDiff line numberDiff line change
@@ -793,8 +793,8 @@ static forceinline int check_bits(const unsigned char *a,
793793
* may be rechecked. It is only passed here as a hint to speed up the check.
794794
* The caller is responsible for not passing an <ignore> value larger than any
795795
* of the two strings. However, referencing any bit from the trailing zero is
796-
* permitted. Equal strings are reported as equal up to and including the last
797-
* zero.
796+
* permitted. Equal strings are reported as a negative number of bits, which
797+
* indicates the end was reached.
798798
*/
799799
static forceinline int string_equal_bits(const unsigned char *a,
800800
const unsigned char *b,
@@ -819,7 +819,7 @@ static forceinline int string_equal_bits(const unsigned char *a,
819819
if (c)
820820
break;
821821
if (!d)
822-
return (beg << 3) + 8; /* equal bytes + zero */
822+
return -1;
823823
}
824824
/* OK now we know that a and b differ at byte <beg>, or that both are zero.
825825
* We have to find what bit is differing and report it as the number of

0 commit comments

Comments
 (0)