From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 411BA469719 for ; Sat, 15 Feb 2020 02:50:06 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <72d0875b-7309-ac79-83c6-51d58f751e6f@tarantool.org> Date: Sat, 15 Feb 2020 00:50:03 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 2/4] b-tree: return NULL on matras_alloc fail List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev , tarantool-patches@dev.tarantool.org 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 /* 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; >