From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 D644C46970F for ; Thu, 28 Nov 2019 02:14:51 +0300 (MSK) References: <84ae817b52e0d9415a0b89c730792b1e0221ff24.1574510839.git.imeevma@gmail.com> <3deea5d5-cc8e-0943-1ad3-07ecc0263088@tarantool.org> <20191127095325.GA10928@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 28 Nov 2019 00:14:48 +0100 MIME-Version: 1.0 In-Reply-To: <20191127095325.GA10928@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Thanks for the fixes! On 27/11/2019 10:53, Mergen Imeev wrote: > 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. No, we should not. I mean, we can, but it is not necessary. We just could stop using malloc() directly in new code. And would drop it from the old code eventually, alongside with other changes. The patchset LGTM. Technically.