From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id kREqJaUrPmrg5xgAWB0awg (envelope-from ) for ; Fri, 26 Jun 2026 03:35:01 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20251104 header.b=efA4kLoR; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 91CF91E098; Fri, 26 Jun 2026 03:35:01 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-5.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,HTML_MESSAGE, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED autolearn=unavailable autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [IPv6:2620:52:6:3111::32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 6B78F1E070 for ; Fri, 26 Jun 2026 03:35:00 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D62534BA2E14 for ; Fri, 26 Jun 2026 07:34:58 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D62534BA2E14 Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20251104 header.b=efA4kLoR Received: from mail-yw1-x112d.google.com (mail-yw1-x112d.google.com [IPv6:2607:f8b0:4864:20::112d]) by sourceware.org (Postfix) with ESMTPS id 433C74BA2E14 for ; Fri, 26 Jun 2026 07:34:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 433C74BA2E14 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 433C74BA2E14 Authentication-Results: sourceware.org; arc=pass smtp.remote-ip=2607:f8b0:4864:20::112d ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1782459268; cv=pass; b=s9RsphIB0XBCICfoyF3+Mrf9YFvJluT841u55QV3M2DUpSLz/xBLGiKPRBoMCqyN0D1B3LCOUCj1h74r2KLF+cUU5rReHvISjzPOHwdvgE5FMU2Mi5fFU4OuZlcNeu5dqaI+3kb1fZBgOCCSQF7YkPvEdGwWnVaRdnuB31kgbDs= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1782459268; c=relaxed/simple; bh=L3Q6NzYX8kK4MsFsuzzJZ3RrDydaKfadhdlTFwf3vII=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=vOrrBB+IHfmkRHMZKOuIFPbi+If32pbjoOScsC3nAPO5lQjxNcyDATpfkiKpHCuCqfxaTBVv827hcRE3KOiJnt5d6xpCvehlukXFaTS/o6PvaVVLB/6qVWGoctoR/8cKtnaBprxbFDgnEOMpV1zkRw0yyjOJfMuVox2IHaiBX3I= ARC-Authentication-Results: i=2; sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20251104 header.b=efA4kLoR DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 433C74BA2E14 Received: by mail-yw1-x112d.google.com with SMTP id 00721157ae682-8001478c58fso520527b3.0 for ; Fri, 26 Jun 2026 00:34:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1782459267; cv=none; d=google.com; s=arc-20260327; b=JF9r8YB+GUl6gQgUNvm9QJesaWuEGOv0JyGWsAgiX+1hZlAKrwzQdfasaPsSsOFfRQ fAP6M4e2WRmlTxaEZ57onjVzJiyyzW24unf53uofs8PAEvGiao2x9Vt0V81fI/9p0xr6 c8h+0HwfgGT/rDxyTkmg7iCOZSURN6kBMtbbSiS13NCIOIgeuyBTaQvoGWHU6cEIRaz3 IDZoMC/xh5IjtTYEKpu4ltaHkxdWVAvc41L41pR3U9wTgiAmGAAu36masbqQsp9EnlI7 qhD1Cvvqvy7SRMGxCxOBlYbtART9Y+zamXOoz4m141/13wIFj0iaTKv4or5Z70JqtUMJ CXsw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=F1mdceQfdVhySBPbkX2wV9H3EXzfP3GdfbXakl7s+Vw=; fh=R1SZKUi+HcalLKhBvYkqs1/+7Rk4SGp5Cp9mU/pfZdQ=; b=W3qu6FqMpZOLBD2Kkr8cTN8XfylhxAykiiY1Rzu8wP6eS0l6KRNrur9QUm8F/Go5UR HIvoc9S1891YhgwWC0+Hh5gZH0wY7llzj4isOmghlEloAOVEDvg+WqCJE6JeWFyDcCt8 V1dMVPxK8R2AKWmS7jZjCkFfOVAQ5+snYtlkL1i12QKesJZLXYc+C4qnd4AU89rSAGAb hrbH75yRAvWZ4Te2xxD8UqHbK1ALvC/fX8PtYJxH7wxHIW3WxAe2HJz1l0dJ0UwUWwWN 1khP1B387GXYKfUrXYKAPQ+tM9AgS+bViXGRnbI63sRUx5mOFcgWEaBqikEcH6wh5T/g j/Fw==; darn=sourceware.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1782459267; x=1783064067; darn=sourceware.org; h=content-type:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:from:to:cc:subject:date:message-id:reply-to :content-type; bh=F1mdceQfdVhySBPbkX2wV9H3EXzfP3GdfbXakl7s+Vw=; b=efA4kLoRTzngZZT8Nh4sU0MT6vpduc6tkKSveDKX2Xxtn9CSmFbvZe7kpw/Qdm5BqR 2Nn1a4XzUyoA1qPF9hrLc4Kn9steK7usqZYwzVos+ihuHW5i5TY4aiwZ5BqbhrnmtuOD o54p+yz7j4xdORkJns5Zo9JithE2s1nHiff1mFd0JETWCzX9x9f+ErPW8QS3yed6NI/K JJuNNTpH181TiH3+vUaRTbfeL+VIZLG6GDmMCIsgK8fpk1G6EU1y3ylufmhOXXSKCiRq zPtlrMOmWrOVgSNQLttc1b65JI7OOdav6Vp+aSbOb4tDkOIvWXwYWaQMzpJqLiGZeSSn 8C0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782459267; x=1783064067; h=content-type:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to:content-type; bh=F1mdceQfdVhySBPbkX2wV9H3EXzfP3GdfbXakl7s+Vw=; b=bg1iKZbk43P1yO1sAmfStqku+LWkBf6cnamdei/QHLl8jzbbQ1mjRWsAQXBjuqSHgN 0ktGgGtV08zfTbOw0ELFCvs12QRoUXfkodTYSA3aZk5A5F8t8xyLvxZk9+DVl30+tklC IbH5Uk1JyFO2YQpklhMqqxBXVFRjt8PlOUcurK5XQ5Mk9/NznQMIflwO+QXPImD0fcMq /22DywCVvcKOkYAGlwq1TZUBOPbNqH+LGGNY+AB5Ss/esJjR8bQhmAf1FERx9Eoxr82y sxVDRKDczPxJJe/ySWt3YKjMbiAykuOERCRliz0d2LPSoLUQUNtSNw1x7eXnyv0EFfdW X77g== X-Gm-Message-State: AOJu0YwxIxxlwU/gAlapP9/hub+gfNL716lR3pDr7qnZDy3chPk6rgie MFr0Xsqv3ZjZLSJkvJ4KDFXx6959LxmpwEm6x1F/ZtUOLv6r8XJddSGQmKv/qFjLo4XrtACpNM3 oz5Gb0r5Hosb6G81LeOEfawQPT9L+InudqfUTGlejFw== X-Gm-Gg: AfdE7cm4dXGPWD5q0meQ80uYKcq0hUZGSb4cED1qXNBWRlxPqsHZRHt0Q14gaX9HSXn 3NxglTFnOmC2khqRrbgLysDSvsGM4BJ2x6jWrFl6n3kT8FzIohMGETi91q6dSPFsg4N6i0KKLVb QNxTQLLXENRAjUYahmosdb3BUGOtB1v2GaN03Di7lUz1vGoGual9+By1hm6MV6TyTqXhlwh6g17 nuRu3ljJkOh8fAz9GnpckpodptHSMToNpMEdaJFFlKbRQNS9KvMjX7YRI8okkNtaKYKUGT/pds= X-Received: by 2002:a05:690c:dd3:b0:7ba:f0d0:5e9b with SMTP id 00721157ae682-80a681fae28mr39382347b3.2.1782459267413; Fri, 26 Jun 2026 00:34:27 -0700 (PDT) MIME-Version: 1.0 References: <20260618180158.2893540-1-firmiana402@gmail.com> <4c790bb0-a1cb-454c-8084-c6e2cfe8e3c9@simark.ca> In-Reply-To: <4c790bb0-a1cb-454c-8084-c6e2cfe8e3c9@simark.ca> From: Firmiana Date: Fri, 26 Jun 2026 15:34:16 +0800 X-Gm-Features: AVVi8Cc4pbkbxlUkxuocH2NOEdJE1Ve0xpjCF8bGXrlapZks2UftYKQprLipuhc Message-ID: Subject: Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands To: Simon Marchi Cc: gdb-patches@sourceware.org Content-Type: multipart/alternative; boundary="00000000000075f9f2065523235c" X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org --00000000000075f9f2065523235c Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, Thanks for the review. Given the amount of changes needed, I plan to prepare a v2 that supersedes this version. Before preparing v2, I would like to clarify a few questions. First, regarding copyright assignment: I do not currently have an FSF copyright assignment on file. I noticed the 2022 binutils announcement that DCO signed contributions are accepted: https://sourceware.org/pipermail/binutils/2022-October/123680.html and binutils/MAINTAINERS documents DCO as an alternative to FSF copyright assignment. Does this DCO path also apply to GDB patches touching gdb/? If so, I will add: Signed-off-by: Jielun Wu to the v2 commit. If GDB still requires FSF copyright assignment for this patch, I am willing to start that process. On the technical comments, I agree with most of them and plan to address them in v2: - make the helper comments more precise; - change the `buf > buf_end` cases in safe_skip_bytes() to assertions; - switch the new fixed-width integer helpers to reference/template style, unless you prefer keeping them closer to the existing safe_read_uleb128/safe_read_sleb128 style; - add assertions that LEN fits in the destination type; - remove the unrelated whitespace-only changes; - change the dwarf_block_to_dwarf_reg_deref guard to the `gdb_assert (buf <=3D buf_end); if (buf =3D=3D buf_end) return -1;` form; - add selftests for dwarf_block_to_dwarf_reg and dwarf_block_to_dwarf_reg_deref; - rewrite the DW_OP_deref* branch using the explicit if/else form you suggested. For the helper style: in v1 I mirrored the nearby safe_read_uleb128 and safe_read_sleb128 helpers, which use output pointer parameters and short "or throw an error" comments. But I agree that these new helpers can be clearer, so unless you prefer otherwise I will use the reference/template style you suggested for v2. Thanks, Jielun On Fri, Jun 26, 2026 at 12:36=E2=80=AFAM Simon Marchi wr= ote: > > > On 2026-06-18 14:01, Jielun Wu wrote: > > Some DWARF expression opcodes read fixed-width operands or block payloa= ds > > from the expression buffer before checking that the bytes are available= . > > With a malformed expression, this can read past the end of the expressi= on > > buffer. > > > > Add helpers that validate fixed-width integer reads and block payload > > skips, and use them in dwarf_expr_context::execute_stack_op. Also guar= d > > the nested DW_OP_entry_value parser before reading the following opcode= . > > > > Add a DWARF assembler test that exercises truncated operands for these > > opcodes. > > > > Tested on x86_64-linux. > > I noted a few comments below. > > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=3D34239 > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=3D34299 > > --- > > gdb/dwarf2/expr.c | 166 +++++++++------ > > .../gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp | 190 ++++++++++++++++++ > > 2 files changed, 300 insertions(+), 56 deletions(-) > > create mode 100644 > gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp > > > > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c > > index c71d4725b03..ead918388a4 100644 > > --- a/gdb/dwarf2/expr.c > > +++ b/gdb/dwarf2/expr.c > > @@ -1370,7 +1370,45 @@ safe_skip_leb128 (const gdb_byte *buf, const > gdb_byte *buf_end) > > error (_("DWARF expression error: ran off end of buffer reading > leb128 value")); > > return buf; > > } > > - > > + > > +/* Helper to skip BYTES bytes or throw an error. */ > > Please precise the comment, "throw an error" in what situation? > > > + > > +static const gdb_byte * > > +safe_skip_bytes (const gdb_byte *buf, const gdb_byte *buf_end, > > + ULONGEST bytes) > > +{ > > + if (buf > buf_end || bytes > (ULONGEST) (buf_end - buf)) > > I would make the "buf > buf_end" condition an assert: > > gdb_assert (buf <=3D buf_end); > > I think that if "buf > buf_end" happens, something would have went wrong > earlier. > > > + error (_("DWARF expression error: ran off end of buffer reading > bytes")); > > + return buf + bytes; > > +} > > + > > +/* Helper to read a fixed-width unsigned integer or throw an error. *= / > > + > > +static const gdb_byte * > > +safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte > *buf_end, > > + int len, bfd_endian byte_order, uint64_t *r) > > Make `r` a reference. > > You appear to use this function most of the time like this: > > op_ptr =3D safe_read_unsigned_integer (op_ptr, op_end, 1, > byte_order, &uoffset); > result =3D uoffset; > > > where `result` is a ULONGEST and uoffset a uint64_t. If you made `r` a > pointer or reference to ULONGEST, wouldn't it make things simpler? You > could just do: > > op_ptr =3D safe_read_unsigned_integer (op_ptr, op_end, 1, > byte_order, result); > > ... and avoid the subsequent assignment. > > But it is sometimes used to read into other types, like cu_offset: > > op_ptr =3D safe_read_unsigned_integer (op_ptr, op_end, 4, > byte_order, &uoffset); > kind_u.param_cu_off =3D (cu_offset) uoffset; > > So we could perhaps make the function templated, like this? > > template > static const gdb_byte * > safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte *buf_en= d, > int len, bfd_endian byte_order, T &r) > { > gdb_assert (len >=3D 0); > gdb_assert (len <=3D sizeof (T)); > > const gdb_byte *data =3D buf; > buf =3D safe_skip_bytes (buf, buf_end, len); > r =3D static_cast (extract_unsigned_integer (data, len, byte_order= )); > return buf; > } > > Then you'll be able to read the integer into whatever destination > integer type directly: > > op_ptr =3D safe_read_unsigned_integer (op_ptr, op_end, 4, byte_or= der, > &kind_u.param_cu_off); > > > +{ > > + gdb_assert (len >=3D 0); > > I would add an assert at len is <=3D the size of the destination type, as > shown above in my example. > > > + const gdb_byte *data =3D buf; > > + buf =3D safe_skip_bytes (buf, buf_end, len); > > + *r =3D extract_unsigned_integer (data, len, byte_order); > > + return buf; > > +} > > + > > +/* Helper to read a fixed-width signed integer or throw an error. */ > > + > > +static const gdb_byte * > > +safe_read_signed_integer (const gdb_byte *buf, const gdb_byte *buf_end= , > > + int len, bfd_endian byte_order, int64_t *r) > > +{ > > + gdb_assert (len >=3D 0); > > + > > + const gdb_byte *data =3D buf; > > + buf =3D safe_skip_bytes (buf, buf_end, len); > > + *r =3D extract_signed_integer (data, len, byte_order); > > + return buf; > > Same comments as above. > > > /* Check that the current operator is either at the end of an > > expression, or that it is followed by a composition operator or by > > @@ -1478,7 +1516,7 @@ dwarf_block_to_dwarf_reg_deref > (gdb::array_view block, > > if (buf =3D=3D NULL) > > return -1; > > if ((int) dwarf_reg !=3D dwarf_reg) > > - return -1; > > + return -1; > > This is an unrelated whitespace change, I will push an obvious patch to > fix it. > > > } > > else > > return -1; > > @@ -1488,6 +1526,8 @@ dwarf_block_to_dwarf_reg_deref > (gdb::array_view block, > > return -1; > > if (offset !=3D 0) > > return -1; > > + if (buf >=3D buf_end) > > + return -1; > > Can this really happen? It seems to me like gdb_read_sleb128 guarantees > that it will not read past buf_end. So it's perhaps possible that `buf > =3D=3D buf_end`, but not `buf > buf_end`. If so, I would do: > > gdb_assert (buf <=3D buf_end); > > if (buf =3D=3D buf_end) > return -1; > > It should be possible to write "selftest" for > dwarf_block_to_dwarf_reg_deref (and dwarf_block_to_dwarf_reg), they look > like pure functions. Can you add that? Look at dwarf2/loc.c for > example to see how selftests are registered, we would do the same in > dwarf2/expr.c. > > > if (*buf =3D=3D DW_OP_deref) > > { > > @@ -1498,7 +1538,7 @@ dwarf_block_to_dwarf_reg_deref > (gdb::array_view block, > > { > > buf++; > > if (buf >=3D buf_end) > > - return -1; > > + return -1; > > Same as above, I'll fix it in an obvious patch. > > > @@ -2016,7 +2066,13 @@ dwarf_expr_context::execute_stack_op > (gdb::array_view expr) > > case DW_OP_deref_type: > > case DW_OP_GNU_deref_type: > > { > > - int addr_size =3D (op =3D=3D DW_OP_deref ? this->m_addr_size = : > *op_ptr++); > > + int addr_size =3D this->m_addr_size; > > + if (op !=3D DW_OP_deref) > > + { > > + op_ptr =3D safe_read_unsigned_integer (op_ptr, op_end, 1, > > + byte_order, &uoffset= ); > > + addr_size =3D uoffset; > > + } > > Please write it with the two branches like this: > > int addr_size; > > if (op =3D=3D DW_OP_deref) > addr_size =3D =3D this->m_addr_size; > else > { > op_ptr =3D safe_read_unsigned_integer (op_ptr, op_end, 1, > byte_order, &uoffset= ); > addr_size =3D uoffset; > } > > Add an empty line after the curly brace that closes the scope. > > Simon > --00000000000075f9f2065523235c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

Thanks for the review. Given the amount of chan= ges needed, I plan to prepare a v2 that
supersedes this version. Before = preparing v2, I would like to clarify a few questions.

First, regard= ing copyright assignment: I do not currently have an FSF
copyright assig= nment on file.=C2=A0 I noticed the 2022 binutils announcement
that DCO s= igned contributions are accepted:

=C2=A0 https://sourceware.org/= pipermail/binutils/2022-October/123680.html

and binutils/MAINTAI= NERS documents DCO as an alternative to FSF copyright
assignment.=C2=A0 = Does this DCO path also apply to GDB patches touching gdb/?
If so, I wil= l add:

=C2=A0 Signed-off-by: Jielun Wu <firmiana402@gmail.com>

to the v2 commit.=C2= =A0 If GDB still requires FSF copyright assignment for
this patch, I am = willing to start that process.

On the technical comments, I agree wi= th most of them and plan to address
them in v2:

=C2=A0 - make the= helper comments more precise;
=C2=A0 - change the `buf > buf_end` ca= ses in safe_skip_bytes() to assertions;
=C2=A0 - switch the new fixed-wi= dth integer helpers to reference/template
=C2=A0 =C2=A0 style, unless yo= u prefer keeping them closer to the existing
=C2=A0 =C2=A0 safe_read_ule= b128/safe_read_sleb128 style;
=C2=A0 - add assertions that LEN fits in t= he destination type;
=C2=A0 - remove the unrelated whitespace-only chang= es;
=C2=A0 - change the dwarf_block_to_dwarf_reg_deref guard to the
= =C2=A0 =C2=A0 `gdb_assert (buf <=3D buf_end); if (buf =3D=3D buf_end) re= turn -1;`
=C2=A0 =C2=A0 form;
=C2=A0 - add selftests= for dwarf_block_to_dwarf_reg and
=C2=A0 =C2=A0 dwarf_block_to_dw= arf_reg_deref;
=C2=A0 - rewrite the DW_OP_deref* branch using the explic= it if/else form you
=C2=A0 =C2=A0 suggested.

For the helper style= : in v1 I mirrored the nearby safe_read_uleb128 and
safe_read_sleb128 he= lpers, which use output pointer parameters and short
"or throw an e= rror" comments.=C2=A0 But I agree that these new helpers can be
cle= arer, so unless you prefer otherwise I will use the reference/template
s= tyle you suggested for v2.

Thanks,
Jielun

On= Fri, Jun 26, 2026 at 12:36=E2=80=AFAM Simon Marchi <simark@simark.ca> wrote:


On 2026-06-18 14:01, Jielun Wu wrote:
> Some DWARF expression opcodes read fixed-width operands or block paylo= ads
> from the expression buffer before checking that the bytes are availabl= e.
> With a malformed expression, this can read past the end of the express= ion
> buffer.
>
> Add helpers that validate fixed-width integer reads and block payload<= br> > skips, and use them in dwarf_expr_context::execute_stack_op.=C2=A0 Als= o guard
> the nested DW_OP_entry_value parser before reading the following opcod= e.
>
> Add a DWARF assembler test that exercises truncated operands for these=
> opcodes.
>
> Tested on x86_64-linux.

I noted a few comments below.

> Bug: https://sourceware.org/bugzilla/sho= w_bug.cgi?id=3D34239
> Bug: https://sourceware.org/bugzilla/sho= w_bug.cgi?id=3D34299
> ---
>=C2=A0 gdb/dwarf2/expr.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 166 +++++++++-= -----
>=C2=A0 .../gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp=C2=A0 | 190 +++++++= +++++++++++
>=C2=A0 2 files changed, 300 insertions(+), 56 deletions(-)
>=C2=A0 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-b= ounds.exp
>
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index c71d4725b03..ead918388a4 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -1370,7 +1370,45 @@ safe_skip_leb128 (const gdb_byte *buf, const gd= b_byte *buf_end)
>=C2=A0 =C2=A0 =C2=A0 error (_("DWARF expression error: ran off end= of buffer reading leb128 value"));
>=C2=A0 =C2=A0 return buf;
>=C2=A0 }
> -=0C
> +
> +/* Helper to skip BYTES bytes or throw an error.=C2=A0 */

Please precise the comment, "throw an error" in what situation?
> +
> +static const gdb_byte *
> +safe_skip_bytes (const gdb_byte *buf, const gdb_byte *buf_end,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ULONGEST bytes)
> +{
> +=C2=A0 if (buf > buf_end || bytes > (ULONGEST) (buf_end - buf))=

I would make the "buf > buf_end" condition an assert:

=C2=A0 gdb_assert (buf <=3D buf_end);

I think that if "buf > buf_end" happens, something would have = went wrong
earlier.

> +=C2=A0 =C2=A0 error (_("DWARF expression error: ran off end of b= uffer reading bytes"));
> +=C2=A0 return buf + bytes;
> +}
> +
> +/* Helper to read a fixed-width unsigned integer or throw an error.= =C2=A0 */
> +
> +static const gdb_byte *
> +safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte *buf_= end,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0int len, bfd_endian byte_order, uint64_t *r)

Make `r` a reference.

You appear to use this function most of the time like this:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 op_ptr =3D safe_read_unsigned_integer (o= p_ptr, op_end, 1,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0byte_order, &uoffset);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 result =3D uoffset;


where `result` is a ULONGEST and uoffset a uint64_t.=C2=A0 If you made `r` = a
pointer or reference to ULONGEST, wouldn't it make things simpler?=C2= =A0 You
could just do:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 op_ptr =3D safe_read_unsigned_integer (o= p_ptr, op_end, 1,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0byte_order, result);

... and avoid the subsequent assignment.

But it is sometimes used to read into other types, like cu_offset:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 op_ptr =3D safe_read_unsigned_int= eger (op_ptr, op_end, 4,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0byte_order, &uoffset);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kind_u.param_cu_off =3D (cu_offse= t) uoffset;

So we could perhaps make the function templated, like this?

=C2=A0 template <typename T>
=C2=A0 static const gdb_byte *
=C2=A0 safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte *buf= _end,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 int len, bfd_endian byte_order, T &r)
=C2=A0 {
=C2=A0 =C2=A0 gdb_assert (len >=3D 0);
=C2=A0 =C2=A0 gdb_assert (len <=3D sizeof (T));

=C2=A0 =C2=A0 const gdb_byte *data =3D buf;
=C2=A0 =C2=A0 buf =3D safe_skip_bytes (buf, buf_end, len);
=C2=A0 =C2=A0 r =3D static_cast<T> (extract_unsigned_integer (data, l= en, byte_order));
=C2=A0 =C2=A0 return buf;
=C2=A0 }

Then you'll be able to read the integer into whatever destination
integer type directly:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 op_ptr =3D safe_read_unsigned_integer (op_ptr, = op_end, 4, byte_order,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0&kind_u.param_cu_off);

> +{
> +=C2=A0 gdb_assert (len >=3D 0);

I would add an assert at len is <=3D the size of the destination type, a= s
shown above in my example.

> +=C2=A0 const gdb_byte *data =3D buf;
> +=C2=A0 buf =3D safe_skip_bytes (buf, buf_end, len);
> +=C2=A0 *r =3D extract_unsigned_integer (data, len, byte_order);
> +=C2=A0 return buf;
> +}
> +
> +/* Helper to read a fixed-width signed integer or throw an error.=C2= =A0 */
> +
> +static const gdb_byte *
> +safe_read_signed_integer (const gdb_byte *buf, const gdb_byte *buf_en= d,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0int len, bfd_endian byte_order, int64_t *r)
> +{
> +=C2=A0 gdb_assert (len >=3D 0);
> +
> +=C2=A0 const gdb_byte *data =3D buf;
> +=C2=A0 buf =3D safe_skip_bytes (buf, buf_end, len);
> +=C2=A0 *r =3D extract_signed_integer (data, len, byte_order);
> +=C2=A0 return buf;

Same comments as above.

>=C2=A0 /* Check that the current operator is either at the end of an >=C2=A0 =C2=A0 =C2=A0expression, or that it is followed by a composition= operator or by
> @@ -1478,7 +1516,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view&= lt;const gdb_byte> block,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (buf =3D=3D NULL)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return -1;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((int) dwarf_reg !=3D dwarf_reg)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0return -1;
> +=C2=A0 =C2=A0 =C2=A0return -1;

This is an unrelated whitespace change, I will push an obvious patch to
fix it.

>=C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 else
>=C2=A0 =C2=A0 =C2=A0 return -1;
> @@ -1488,6 +1526,8 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view&= lt;const gdb_byte> block,
>=C2=A0 =C2=A0 =C2=A0 return -1;
>=C2=A0 =C2=A0 if (offset !=3D 0)
>=C2=A0 =C2=A0 =C2=A0 return -1;
> +=C2=A0 if (buf >=3D buf_end)
> +=C2=A0 =C2=A0 return -1;

Can this really happen?=C2=A0 It seems to me like gdb_read_sleb128 guarante= es
that it will not read past buf_end.=C2=A0 So it's perhaps possible that= `buf
=3D=3D buf_end`, but not `buf > buf_end`.=C2=A0 If so, I would do:

=C2=A0 gdb_assert (buf <=3D buf_end);

=C2=A0 if (buf =3D=3D buf_end)
=C2=A0 =C2=A0 return -1;

It should be possible to write "selftest" for
dwarf_block_to_dwarf_reg_deref (and dwarf_block_to_dwarf_reg), they look like pure functions.=C2=A0 Can you add that?=C2=A0 Look at dwarf2/loc.c for=
example to see how selftests are registered, we would do the same in
dwarf2/expr.c.

>=C2=A0 =C2=A0 if (*buf =3D=3D DW_OP_deref)
>=C2=A0 =C2=A0 =C2=A0 {
> @@ -1498,7 +1538,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view&= lt;const gdb_byte> block,
>=C2=A0 =C2=A0 =C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 buf++;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (buf >=3D buf_end)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0return -1;
> +=C2=A0 =C2=A0 =C2=A0return -1;

Same as above, I'll fix it in an obvious patch.

> @@ -2016,7 +2066,13 @@ dwarf_expr_context::execute_stack_op (gdb::arra= y_view<const gdb_byte> expr)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case DW_OP_deref_type:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0case DW_OP_GNU_deref_type:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int addr_size =3D (op =3D=3D DW_OP_= deref ? this->m_addr_size : *op_ptr++);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int addr_size =3D this->m_addr_s= ize;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (op !=3D DW_OP_deref)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0op_ptr =3D safe_read_= unsigned_integer (op_ptr, op_end, 1,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 byte_order, &uoffset);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0addr_size =3D uoffset= ;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}

Please write it with the two branches like this:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int addr_size;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (op =3D=3D DW_OP_deref)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0addr_size =3D =3D th= is->m_addr_size;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 op_ptr =3D safe_rea= d_unsigned_integer (op_ptr, op_end, 1,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0byte_order, &uoffset);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 addr_size =3D uoffs= et;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }

Add an empty line after the curly brace that closes the scope.

Simon
--00000000000075f9f2065523235c--