From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 1C3FC46970F for ; Wed, 27 Nov 2019 12:53:28 +0300 (MSK) Date: Wed, 27 Nov 2019 12:53:25 +0300 From: Mergen Imeev Message-ID: <20191127095325.GA10928@tarantool.org> References: <84ae817b52e0d9415a0b89c730792b1e0221ff24.1574510839.git.imeevma@gmail.com> <3deea5d5-cc8e-0943-1ad3-07ecc0263088@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <3deea5d5-cc8e-0943-1ad3-07ecc0263088@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Thank you for review! My answer and diff between patches below. On Sun, Nov 24, 2019 at 04:27:58PM +0100, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > > diff --git a/src/box/session.cc b/src/box/session.cc > > index 461d1cf..93d8b42 100644 > > --- a/src/box/session.cc > > +++ b/src/box/session.cc > > @@ -338,3 +341,131 @@ generic_session_sync(struct session *session) > > + > > +struct iterator * > > +session_options_create_iterator(struct index *index, enum iterator_type type, > > + const char *key, uint32_t part_count) > > +{ > > + char *decoded_key = NULL; > > + if (part_count > 0) { > > + assert(part_count == 1); > > + assert(mp_typeof(*key) == MP_STR); > > + uint32_t len; > > + const char *name = mp_decode_str(&key, &len); > > + decoded_key = (char *)malloc(len + 1); > > + if (decoded_key == NULL) { > > + diag_set(OutOfMemory, len + 1, "malloc", "decoded_key"); > > + return NULL; > > + } > > + memcpy(decoded_key, name, len); > > + decoded_key[len] = '\0'; > > + } > > + struct space *space = space_cache_find(index->def->space_id); > > + struct session_options_iterator *it = > > + (session_options_iterator *)malloc(sizeof(*it)); > > + if (it == NULL) { > > decoded_key leaks here. But heap OOM is such an unlikely event, > and is forgotten for be checked so often, that perhaps we should > reconsider moving this https://github.com/tarantool/tarantool/issues/3534 > out of wish list and start using panic in new places. Probably > wrapped into a 'heap_alloc()' function (name is discussable). > > I think, Kirill won't be against this. Fixed. I asked about issue #3534 in a scrum, but haven’t received a definite answer yet. I will ask about it again, but a little later, since now we have to prepare for release 2.3.1. Also, it looks like we should replace all malloc() checks in one commit. diff --git a/src/box/session.cc b/src/box/session.cc index 93d8b42..fbaabf0 100644 --- a/src/box/session.cc +++ b/src/box/session.cc @@ -454,6 +454,7 @@ session_options_create_iterator(struct index *index, enum iterator_type type, (session_options_iterator *)malloc(sizeof(*it)); if (it == NULL) { diag_set(OutOfMemory, sizeof(*it), "malloc", "it"); + free(decoded_key); return NULL; } int current_id = iterator_type_is_reverse(type) ?