From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f196.google.com (mail-lj1-f196.google.com [209.85.208.196]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5B75646970E for ; Tue, 4 Feb 2020 21:25:44 +0300 (MSK) Received: by mail-lj1-f196.google.com with SMTP id h23so19672580ljc.8 for ; Tue, 04 Feb 2020 10:25:44 -0800 (PST) Date: Tue, 4 Feb 2020 21:25:41 +0300 From: Konstantin Osipov Message-ID: <20200204182541.GA6876@atlas> References: <591f17842bd4138b5598d6d69822195daef63375.1579541242.git.i.kosarev@tarantool.org> <20200121103258.GG82780@tarantool.org> <20200131081855.GB10740@atlas> <20200204171342.GC62548@tarantool.org> <20200204172538.GA7665@atlas> <20200204180853.GA78702@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200204180853.GA78702@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org * Nikita Pettik [20/02/04 21:10]: > On 04 Feb 20:25, Konstantin Osipov wrote: > > * Nikita Pettik [20/02/04 20:21]: > > > > > On 20 Jan 21:13, Ilya Kosarev wrote: > > > > > > In bps_tree_create_leaf we use matras_alloc in case > > > > > > 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. > > > > > > > > I don't understand the attempt to fix it. > > > > > > > > The reason the allocations are not checked - most likely -because > > > > BPS should refuse to even begin an operation if there is not > > > > enough memory in matras. > > > > > > According to the code it doesn't look so. Matras allocation checked > > > on fails, except two ones in bps_tree_create_inner() and > > > bps_tree_create_leaf(). > > > > Because reserve_blocks() should reserve enough blocks or fail, and > > it is called before create_inner/create_leaf in all execution paths? > > Consider following path: > > memtx_tree_index_replace > | > ->memtx_tree_insert (bps_tree_insert) > | > ->bps_tree_insert_first_elem > | > ->bps_tree_create_leaf > | > -> matras_alloc > > In this case reserve_blocks() is not called. AFAIU path is likely to > be reachable. Am I missing smth? No, in this trace I agree with you, we clearly see some sort of refactoring artefact or a coding bug. What I am observing is that bps_tree_insert_first_elem() actually checks the return value of bps_tree_create_leaf(). But bps_tree_create_leaf() never returns NULL, so this check is never false. bps was originally written in C++, so all of these checks for OOm were added when it was rewritten in C, after the fact. Looks like we're dealing with an artefact of this rewrite - the checks are not consistent. -- Konstantin Osipov, Moscow, Russia https://scylladb.com