[Tarantool-patches] [PATCH v4 2/4] b-tree: return NULL on matras_alloc fail
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Feb 15 02:50:03 MSK 2020
Thanks for the patch!
See 3 comments below.
On 14/02/2020 20:39, Ilya Kosarev wrote:
> In bps_tree_create_leaf we use matras_alloc in case
1. This is not only about bps_tree_create_leaf() anymore. You
also patched bps_tree_create_inner().
> bps_tree_garbage_pop didn't work out. However it also might not
> succeed. Then we need to return NULL instead of dereferencing NULL
> pointer.
>
> Part of #3807
> ---
> src/lib/salad/bps_tree.h | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h
> index d28b53f53..d71340143 100644
> --- a/src/lib/salad/bps_tree.h
> +++ b/src/lib/salad/bps_tree.h
> @@ -3710,6 +3718,8 @@ bps_tree_process_insert_leaf(struct bps_tree *tree,
> bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
> struct bps_inner *new_root = bps_tree_create_inner(tree,
> &new_root_id);
> + if (!new_root)
> + return -1;
2. Here are two problems.
1) This can't fail, because above there is bps_tree_reserve_blocks()
invocation. Looks like it reserves enough blocks (but I propose you
to check it, I don't know).
2) Even if that fails, the tree to that moment is already partially
changed. And if you return -1 here, it is corrupted. To test that I
added an error injection to bps_tree_process_insert_leaf() in this
place. Initially it was false. I called box.cfg{}. Then I set it
true. And created a space, an index. Tried to insert 100 numbers.
It failed due to OOM. Then I turned the errinj off again, inserted
the values again, and made a select. I got broken order of the
select, and values returned multiple times.
Here is what I have done:
================================================================================
--- a/src/lib/salad/bps_tree.h
+++ b/src/lib/salad/bps_tree.h
@@ -38,6 +38,8 @@
#include <stdio.h> /* printf */
#include "small/matras.h"
+extern int bps_errinj;
+
/* {{{ BPS-tree description */
/**
* BPS-tree implementation.
@@ -3716,6 +3718,8 @@ bps_tree_process_insert_leaf(struct bps_tree *tree,
leaf_path_elem, &new_path_elem, mc1, new_elem);
bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
+ if (bps_errinj)
+ return -1;
struct bps_inner *new_root = bps_tree_create_inner(tree,
&new_root_id);
if (!new_root)
================================================================================
Also you need to declare bps_errinj somewhere in a .c file.
Here is the test:
box.cfg{}
-- In lldb/gdb make bps_errinj variable 1.
s = box.schema.create_space('test')
_ = s:create_index('pk')
for i = 1, 100 do s:replace{i} end
-- This was an error. Now make bps_errinj 0 again.
for i = 1, 100 do s:replace{i} end
s:select{}
The select will return tuples in a wrong order.
> new_root->header.size = 2;
> new_root->child_ids[0] = tree->root_id;
> new_root->child_ids[1] = new_block_id;
> @@ -4030,6 +4042,8 @@ bps_tree_process_insert_inner(struct bps_tree *tree,
> bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
> struct bps_inner *new_root =
> bps_tree_create_inner(tree, &new_root_id);
> + if (!new_root)
> + return -1;
3. The same problem is here. You can't return from now on, the
tree is changed already.
> new_root->header.size = 2;
> new_root->child_ids[0] = tree->root_id;
> new_root->child_ids[1] = new_block_id;
>
More information about the Tarantool-patches
mailing list