From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailsec117.isp.belgacom.be (mailsec117.isp.belgacom.be [195.238.20.113]) by sourceware.org (Postfix) with ESMTPS id 00C21385220F for ; Mon, 21 Nov 2022 21:22:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 00C21385220F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=skynet.be Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=skynet.be DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=skynet.be; i=@skynet.be; q=dns/txt; s=rmail; t=1669065751; x=1700601751; h=message-id:subject:from:to:date:in-reply-to:references: mime-version:content-transfer-encoding; bh=digdMVjx02B78m89QKaZZ0pTrI5w2e8XVtOo2hSR2SI=; b=juVoDiEtLoqnUfExTnWokZRbiqvZvETSmSujs94ExBM1PBVFgp1pg7D8 5E5pFb1nE+ClmcKl8ZC/u2kAUx+BuV7m4zj5oaXvhqKwMax3xBYo/Uz4A KHzxPfo8CCnGHJ3Nl51lLtAFvn8FwlyPM5fw+IqKf1Akd61LE+ccrxQpK 8=; X-ExtLoop: 1 X-IPAS-Result: =?us-ascii?q?A2CuAQA363tj/1uGgG0NTYEJCYZPhE6RGgOfLw8BAQEBA?= =?us-ascii?q?QEBAQEJRAQBAYUFAoUBJjgTAQIEAQEBAQMCAwEBAQEBAQMBAQYBAQEBAQEGB?= =?us-ascii?q?AGBG4Uvgnsig3wBAQEBAgEjBAsBVgsOCgICJgICVwYBEoV9syt6fzOBAYRxm?= =?us-ascii?q?mWBZ4EULIkAg2CEMDeBVUSBFYJ6MD6IG4JnBJd0HDcDGSsdQAMLbQpKG1gOC?= =?us-ascii?q?R8cDhcNBQYSAyBsBQo3DygvZxAbHBsHgQwqKBUDBAQDAgYTAyACDSkxFAQpE?= =?us-ascii?q?w0rJ28JAgMiZQUDAwQoLAMJIR8HFhEkPAdWOgEEAwIPIDgGAwkDAiQTQ3UuE?= =?us-ascii?q?RUFAwsVJQgFSwQIOQUGUhICChEDEg8GJkYOSD45FgYndA4OFANegWkENYFxC?= =?us-ascii?q?pxJAQYnLDUUgR0TezWNDIUlLq9UNAeDa4FHBgyXcYcKMqkWLZYSdSCnWIF5g?= =?us-ascii?q?X5tgzpSKI4sFo4vdDsCBwEKAQEDCYoDXwEB?= IronPort-PHdr: A9a23:ACnDqh1cWX6nev7UsmDO2AMyDhhOgF0UFjAc5pdvsb9SaKPrp82kY BaDo6gzxwaVA82bs6sC17CN9fi4GCQp2tWoiDg6aptCVhsI2409vjcLJ4q7M3D9N+PgdCcgH c5PBxdP9nC/NlVJSo6lPwWB6nK94iQPFRrhKAF7Ovr6GpLIj8Swyuu+54Dfbx9HiTajbr5+N hW7oATeusULjoZvK7s6xh/VrndVYehbyn1kKFyJkxrg+su8+YNo/jhNtf4m68NOS7jxcb4iT bxfAjQmMmQ169PuuBLeUwaB5WYSX3sPnBZQDAfL8B/1XpHqsivnreV9wzWVPdf3Tb8vRzuv6 bpgRQLyhycGMz4593zXitB1galGrh+tuwBzzojJa4yTKfFwfL7SfckCSGRCQ8hfVzJPDI2+Y IsBE+UOM+lXoYbmqlsSrhazHxWgCP/1xzJKgHL9wK000/4mEQHDxAEuBdIOsHXQrNX0LKcSS f66x7TIwjrZdfNW3i796JXJch8/u/GMRq97fM3JyUkvFgPFilSQqZT9PzyLzOgMvXKU7/BnV eK2lm4nqh9+oiK0xso3kYnJhYIUylba+iVl2oo6PsO3RVd9bNW5H5ReqzuUOJFqQsM+XW5oo iA6x6UHtJO7ciYEx4oryhzQZvCZb4SG7RDuWeaRLDtlmn9pZbCyigiv/US+1+HxSMm53UpWo ydEnNTAqH4A2wLX58SbTPZ240Sv2S6X2gzO9u1JJVo4mbfFJ5Mi2LI8i5QevEvZEiLyhUn7i rKdeF8+9eiy8evnZ63rppqbN4BplA7zKr8umsmjAeQgNQgOQnSb9fy81LL9+U35R61Hgfkrn qTdrpzWP9kXq6+6DgNPz4ov8QuzAjOg39gAnHkHKkxKeA6fgoT0J13DJOr0Aeqhj1mvijtn2 v7LM7L7DpjJM3TPiLLhcqx8605Yxgoz19df55dMB7EZIPLzW0vxtN3ADh8hLQO42ejmB89n1 oMZQGKPH66ZP73IsV+T/e8vOOqMZY8TuDnjN/gp/eXhgmEhlV8bZqamxYEXZ2ygHvR6P0WZZ mLhjskZHWcQogU+VPDqiEGFUTNLfHayXqc86SshCIKlForDXYGtgLmb0yehHZ1afHxJBUqUH Xfya4qEQ+sMaD6VIsJ5nTwLSKOuR5Y51R60qA/117pnIfTP+iADrpLj08V15/fPmh4s+z17F d6d3H+XQ2FzhGMISGx+4Kcqmk14zVHL9KFiiPxZXYhI5vZASAQSLp/ZxvZ5Tdv7DEaJcduDD U6vXt6mDCoZVdUs2dQDfE9nFpOllB+Q8TCtBuotl72PBYQs/+rj1mL2Ptt8xmzdnP05j1giQ 9NXOCu5j7R47hXSCpTSu16ahqCnaeIW0Xiepy+40WOSsRQAA0ZLWqLfUCVHDnY= IronPort-Data: A9a23:GaUQiqhyy9wSCTQ6m9bGUCQwX161BxEKZh0ujC45NGQN5FlHY01je htvWDvTb/7fYjT8LYtwYIy3px4H75WAnNFiHgNoqXpnEy9jpJueD7x1DKtQ0wB+jyHnZBg6h ynLQoCYdKjYdleF+lH3dOGJQUBUjcmgXqD7BPPPJhd/TAplTDZJoR94kobVuKYx6TSCK17L6 I2aT/H3Ygf/gWctaDtMscpvlTs21BjMkGJA1rABTa0T1LPuvyF9JI4SI6i3M0z5TuF8dgJtb 7+epF0R1jqxEyYFUrtJoJ6iGqE5auK60Ty1t5Zjc/PKbi6uCcAF+v1T2PI0MS+7gtgS9jx74 I0lWZeYEW/FMkBQ8QgQe0EwLs1wAUFJ0LjpJii47t2t9UL5b1LpmO1EPGsTZbRNr46bAUkWn RAZAANUP0rF3rzmhuv9E7hZ7ighBJCzbcVG4CEmlGqFS6d/KXzAa/yiCdtwxDcxgsFWBfuYe MMDbiNybRnaeDVUOUYRBY54lurAanzXKmEG9gzN//Jni4TV5A1b6bO8aNGSQYWXScoPhUux+ V/p/HusV3n2M/Tak1Jp6EmEne7KlDn4cJkTEbSi9bhhiTW7yWAZTQUfSVC7rOKRkUmjQdlSN EUO92wpt6dayaCwZoikGUTj/Dvd5E5ZAoIBVeQ28wXIxrvSpQeCHi4OQzpOYdchsYk8SFTGy 2O0oj8gPhQ32JX9dJ5X3u78Qe+aUcTeEYPOieLog+fIDxkPbbzfVi7yc+s= IronPort-HdrOrdr: A9a23:OhoPqq5E617VKeesfwPXwN3XdLJyesId70hD6qkXc20xTiX4ra CTdZsguiMc5Ax9ZJhko7690cq7MBHhHPxOirX5VI3KNDUO+lHIEGgI1+HfKlPbdxEWutQttp tdTw== X-IronPort-Anti-Spam-Filtered: true X-ProximusIPWarmup: true Received: from 91.134-128-109.adsl-dyn.isp.belgacom.be (HELO [192.168.1.19]) ([109.128.134.91]) by relay.proximus.be with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Nov 2022 22:22:29 +0100 Message-ID: <1ccc9ab435e713ebf170b5e19db0633a2c0a1875.camel@skynet.be> Subject: Re: [RFA] Fix use after free introduced by $_hit_bpnum/$_hit_locno variables. From: Philippe Waroquiers To: Simon Marchi , gdb-patches@sourceware.org Date: Mon, 21 Nov 2022 22:22:28 +0100 In-Reply-To: <24e42ada-2c7a-208f-4783-972b3cb852ce@simark.ca> References: <20221120104406.2134561-1-philippe.waroquiers@skynet.be> <24e42ada-2c7a-208f-4783-972b3cb852ce@simark.ca> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.3-1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, 2022-11-21 at 10:10 -0500, Simon Marchi wrote: > > On 11/20/22 05:44, Philippe Waroquiers via Gdb-patches wrote: > > If the commands of the bpstat bs contain commands such as step or next or > > continue, the BS and its commands are freed by execute_control_command. > > > > So, we cannot remember the BS that was printed. Instead, remember > > the bpnum and locno. > > > > Regtested on debian/amd64 and re-run a few tests under valgrind. > > --- > >  gdb/breakpoint.c | 62 ++++++++++++++++++++++++++++-------------------- > >  1 file changed, 36 insertions(+), 26 deletions(-) > > > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > > index 7f6400db624..5b691673a0e 100644 > > --- a/gdb/breakpoint.c > > +++ b/gdb/breakpoint.c > > @@ -4574,18 +4574,17 @@ command_line_is_silent (struct command_line *cmd) > >    return cmd && (strcmp ("silent", cmd->line) == 0); > >  } > > > > -/* Sets the $_hit_bpnum and $_hit_locno to the bpnum and locno of bs. */ > > +/* Sets the $_hit_bpnum and $_hit_locno to bpnum and locno. > > + A locno 0 is changed to 1 to e.g. let the user do > > + (gdb) disable $_hit_bpnum.$_hit_locno > > + for a single location breakpoint. */ > > + > >  static void > > -set_hit_convenience_vars (bpstat *bs) > > +set_hit_convenience_vars (int bpnum, int locno) > >  { > > - const struct breakpoint *b = bs->breakpoint_at; > > - if (b != nullptr) > > - { > > - int locno = bpstat_locno (bs); > > - set_internalvar_integer (lookup_internalvar ("_hit_bpnum"), b->number); > > - set_internalvar_integer (lookup_internalvar ("_hit_locno"), > > - (locno > 0 ? locno : 1)); > > - } > > + set_internalvar_integer (lookup_internalvar ("_hit_bpnum"), bpnum); > > + set_internalvar_integer (lookup_internalvar ("_hit_locno"), > > + (locno > 0 ? locno : 1)); > > I haven't had time to look at the original patch (sorry, just so many > things to do), but the naming caught my attention. We have both "num" > and "no" as abbreviations for "number". It looks a bit inconsistent. > Was it on purpose? I don't so much about the variable names, but the > names exposed to the user. There was no particular specific reason. The names were first $bkptno and $locno. Tom gave comments about the fact that new conv vars should start with $_ and that $bkptno was not consistent with $bpnum. We need 2 different names to avoid making an incompatible change to $bpnum behaviour. As the semantic of $bkptno and $locno was not clear from the names, these were changed to $_hit_bpnum (where bpnum trailer was now chosen on purpose to be consistent in terminology with the $bpnum set by the "breakpoint" command). GDB (e.g. user manual and/or code) uses various conventions for "integer" identifying something (such as "thread ID", TASKNO, INFNO or bkptno that was already used in the mi interface).  "locno" terminology was used because the trailing "no" seems used relatively frequently. This can of course still be changed if deemed better. > > But the fix LGTM: > > Approved-By: Simon Marchi > > Simon Thanks for the review, pushed. Philippe