From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3248 invoked by alias); 28 Jun 2018 09:31:29 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 3236 invoked by uid 89); 28 Jun 2018 09:31:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:may, H*r:forged, HContent-Transfer-Encoding:8bit X-HELO: relay.fit.cvut.cz Received: from relay.fit.cvut.cz (HELO relay.fit.cvut.cz) (147.32.232.237) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 28 Jun 2018 09:31:25 +0000 Received: from imap.fit.cvut.cz (imap.fit.cvut.cz [IPv6:2001:718:2:2901:0:0:0:238] (may be forged)) by relay.fit.cvut.cz (8.15.2/8.15.2) with ESMTPS id w5S9VFka089843 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=OK); Thu, 28 Jun 2018 11:31:16 +0200 (CEST) (envelope-from jan.vrany@fit.cvut.cz) Received: from sao (0279192e.bb.sky.com [2.121.25.46] (may be forged)) (authenticated bits=0 as user vranyj1) by imap.fit.cvut.cz (8.15.2/8.15.2) with ESMTPSA id w5S9VD1j053575 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT); Thu, 28 Jun 2018 11:31:14 +0200 (CEST) (envelope-from jan.vrany@fit.cvut.cz) Message-ID: Subject: Re: [PATCH v2] Fix segfault when invoking -var-info-path-expression on a dynamic varobj From: Jan Vrany To: Simon Marchi , gdb-patches@sourceware.org Date: Thu, 28 Jun 2018 09:31:00 -0000 In-Reply-To: <93b2a663-2672-10c0-097d-9a9026fa4e00@simark.ca> References: <20180625205901.29762-1-jan.vrany@fit.cvut.cz> <93b2a663-2672-10c0-097d-9a9026fa4e00@simark.ca> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-SW-Source: 2018-06/txt/msg00677.txt.bz2 On Tue, 2018-06-26 at 22:24 -0400, Simon Marchi wrote: > On 2018-06-25 04:59 PM, Jan Vrany wrote: > > Invoking -var-info-path-expression on a dynamic varobj lead either > > in wrong > > (nonsense) result or to a segmentation fault in > > cplus_describe_child(). > > This was caused by the fact that varobj_get_path_expr() called > > cplus_path_expr_of_child() ignoring the fact the parent of the > > variable > > is dynamic. Then, cplus_describe_child() accessed the underlaying C > > type > > members by index, causing (i) either wrong (nonsense) expression > > being > > returned (since dynamic child may be completely arbibtrary value) > > or (ii) segmentation fault (in case the index higher than number of > > underlaying C type members. > > > > This fixes the problem by checking whether a varobj is a child of a > > dynamic > > varobj and, if so, reporting an error to MI consumer as described > > in > > the documentation. > > Hi Jan, > > The patch does not compile for me: > > CXX varobj.o > In file included from /home/simark/src/binutils- > gdb/gdb/common/common-defs.h:90, > from /home/simark/src/binutils-gdb/gdb/defs.h:28, > from /home/simark/src/binutils-gdb/gdb/varobj.c:18: > /home/simark/src/binutils-gdb/gdb/varobj.c: In function ‘const char* > varobj_get_path_expr(const varobj*)’: > /home/simark/src/binutils-gdb/gdb/varobj.c:969:41: error: ‘cur’ was > not declared in this scope > gdb_assert (!varobj_is_dynamic_p (cur)); > ^~~ > /home/simark/src/binutils-gdb/gdb/common/gdb_assert.h:33:13: note: in > definition of macro ‘gdb_assert’ > ((void) ((expr) ? 0 > : \ > Hi Simon, argh, sorry. Forgotten "git add". Will fix. > ^~~~ > > diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c > > index 38c59c7e66..fa47387357 100644 > > --- a/gdb/mi/mi-cmd-var.c > > +++ b/gdb/mi/mi-cmd-var.c > > @@ -438,7 +438,15 @@ mi_cmd_var_info_path_expression (const char > > *command, char **argv, int argc) > > > > /* Get varobj handle, if a valid var obj name was specified. */ > > var = varobj_get_handle (argv[0]); > > - > > + > > + /* -var-info-path-expression is currently not valid for children > > of > > + a dynamic varobj. */ > > + for (struct varobj *cur = var->parent; cur != nullptr; cur = > > cur->parent) > > + { > > + if (varobj_is_dynamic_p (cur)) > > + error (_("Invalid variable object (child of a dynamic > > varobj)")); > > + } > > + > > To make it simpler, instead of checking all the parents recursively > here, have you > considered adding a check to the varobj_get_path_expr_parent, like > this? I haven't > given it a very long though, but it does pass your test :). > > diff --git a/gdb/varobj.c b/gdb/varobj.c > index f2c10ddc57ff..e601cf0b9780 100644 > --- a/gdb/varobj.c > +++ b/gdb/varobj.c > @@ -948,6 +948,9 @@ varobj_get_path_expr_parent (const struct varobj > *var) > while (!is_root_p (parent) && !is_path_expr_parent (parent)) > parent = parent->parent; > > + if (varobj_is_dynamic_p (parent)) > + error (_("Invalid variable object (child of a dynamic > varobj)")); > + > return parent; > } > No, I have not considered this - still learning internals. Generally I prefer to validate input as soon as possible rather than go on and fail later but I agree it's a matter of taste. However, varobj_get_path_expr_parent() is indeed much better place for the assert (the one that failed to compile :), something like: diff --git a/gdb/varobj.c b/gdb/varobj.c index 02441410e8..050c240bef 100644 --- a/gdb/varobj.c +++ b/gdb/varobj.c @@ -948,6 +948,10 @@ varobj_get_path_expr_parent (const struct varobj *var) while (!is_root_p (parent) && !is_path_expr_parent (parent)) parent = parent->parent; + /* Computation of full rooted expression for children of dynamic + varobjs is not supported. */ + gdb_assert (!varobj_is_dynamic_p (parent)); + return parent; } What do you think? I'll send a new version once we decide. Thanks! Jan