From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0F23121A10 for ; Tue, 25 Dec 2018 12:26:32 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ybk76BPSvxnw for ; Tue, 25 Dec 2018 12:26:31 -0500 (EST) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id BD41021A04 for ; Tue, 25 Dec 2018 12:26:31 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH v1 2/3] sql: fix sql_vdbe_mem_alloc_region result memory From: "n.pettik" In-Reply-To: Date: Tue, 25 Dec 2018 19:26:29 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: References: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov > The function sql_vdbe_mem_alloc_region that constructing the > value of Vdbe Mem object used to change only flags responsible > for it's type. > It is also required to grind old flags, as their combination may > be invalid. > In a typical Vdbe scenario, OP_MakeRecord and OP_RowData make > memory release with sqlite3VdbeMemRelease and allocation > on region with sql_vdbe_mem_alloc_region call. An integrity > assert based on sqlite3VdbeCheckMemInvariants would fire here > because of contradictory combination of flags > MEM_Static | (MEM_Blob | MEM_Ephem). >=20 > Needed for #3850 > --- > src/box/sql/vdbeaux.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) >=20 > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index fc805e3aa..d477662a4 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -3231,7 +3231,8 @@ sql_vdbe_mem_alloc_region(Mem *vdbe_mem, = uint32_t size) > vdbe_mem->z =3D region_alloc(&fiber()->gc, size); > if (vdbe_mem->z =3D=3D NULL) > return SQLITE_NOMEM; > - MemSetTypeFlag(vdbe_mem, MEM_Blob | MEM_Ephem); > + vdbe_mem->flags =3D MEM_Ephem | MEM_Blob; > + assert(sqlite3VdbeCheckMemInvariants(vdbe_mem)); > return SQLITE_OK; > } I=E2=80=99ve changed your commit message a bit: Function sql_vdbe_mem_alloc_region() that constructs the value of Vdbe Mem object used to change only type related flags. However, it is also required to erase other flags (for instance flags related to allocation policy: static, dynamic etc), since their combination may be invalid. In a typical Vdbe scenario, OP_MakeRecord and OP_RowData release memory with sqlite3VdbeMemRelease() and allocate on region with sql_vdbe_mem_alloc_region(). An integrity assert based on sqlite3VdbeCheckMemInvariants() would fire here due to incompatible combination of flags: MEM_Static | (MEM_Blob | MEM_Ephem). Read it and in case it looks OK to you, please apply. LGTM.