From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id +LtBLx5ZPWqDDBgAWB0awg (envelope-from ) for ; Thu, 25 Jun 2026 12:36:46 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1782405406; bh=CpI81R2d1nONzlUaRRXZw4OeJj2q6n+bHZvD8pt6f9E=; h=Date:Subject:To:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=mnCf+piSQJTx4hlM1wsEz2gFDQd8PmRfPVH0V254wRFuyFnmAw89/uURbLdCpfKFB z7SrAcTEYR0zoAUYkamhki6JPwU8p67B17AacreWo3LAviZHlQQmqfUt53e33uZY84 WwWFiYYRr9WfZratCWWOQLVm6CY4gjum8czeBC3o= Received: by simark.ca (Postfix, from userid 112) id B55E11E098; Thu, 25 Jun 2026 12:36:46 -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,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.1 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=fY9Sm0Rz; dkim-atps=neutral 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 DE29B1E024 for ; Thu, 25 Jun 2026 12:36:45 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 275D34BA23DB for ; Thu, 25 Jun 2026 16:36:44 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 275D34BA23DB Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=fY9Sm0Rz Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 0C3234BA2E15 for ; Thu, 25 Jun 2026 16:36:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0C3234BA2E15 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 0C3234BA2E15 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1782405378; cv=none; b=o8jhkh/rUrb2o2JhG9Lln5fc48ASNVzgXvq5LoUXEnAipADDVClU42r0VVzhJXSLhg8Ga9E/vfQW004sNyBVo4eMSVas1/B2juRcdBSPkGHZVrRH7Wq/z7kIa19/xboUzvSNI9mPjQr2GLQ5p1iq+EYOOu/3Z19Ww+UH5DALXJw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1782405378; c=relaxed/simple; bh=CpI81R2d1nONzlUaRRXZw4OeJj2q6n+bHZvD8pt6f9E=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=LePgfeidWoPU9ryhbnio6vbj5j0z23sZMIBL2fMULaLcFTSJYZyK8hcHgOxNIiY8F4/kkXdHRV3IeTu8TnIc9dlTgu4/HX/P7bSw3rvIWVcU73SOmvbTVUaNkutwkK5Ockmt7TYdEw/OKjq+NhA22H7iMtqk4xlGsv6qPawaQ3Q= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=fY9Sm0Rz DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0C3234BA2E15 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1782405376; bh=CpI81R2d1nONzlUaRRXZw4OeJj2q6n+bHZvD8pt6f9E=; h=Date:Subject:To:References:From:In-Reply-To:From; b=fY9Sm0RzUTftgzj0acj6eyyJxZYX33auQAWNfpDMx0FKmfIYG32V+0rEh6IBBBgMz 2pbTIDbMslr0D32tsrJXYu9r+tmdpdJGeN1T+KhTOE2US+Es94PR4DxhPfqp0GFo6V EDtJMla02WEY8m5lEUb7sjlxgwXODoDZ5B3Oy5RE= Received: by simark.ca (Postfix) id 37C1B1E024; Thu, 25 Jun 2026 12:36:15 -0400 (EDT) Message-ID: <4c790bb0-a1cb-454c-8084-c6e2cfe8e3c9@simark.ca> Date: Thu, 25 Jun 2026 12:36:15 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands To: Jielun Wu , gdb-patches@sourceware.org References: <20260618180158.2893540-1-firmiana402@gmail.com> Content-Language: en-US From: Simon Marchi In-Reply-To: <20260618180158.2893540-1-firmiana402@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 On 2026-06-18 14:01, Jielun Wu wrote: > Some DWARF expression opcodes read fixed-width operands or block payloads > from the expression buffer before checking that the bytes are available. > With a malformed expression, this can read past the end of the expression > buffer. > > Add helpers that validate fixed-width integer reads and block payload > skips, and use them in dwarf_expr_context::execute_stack_op. Also guard > 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=34239 > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34299 > --- > 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 <= 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 = safe_read_unsigned_integer (op_ptr, op_end, 1, byte_order, &uoffset); result = 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 = 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 = safe_read_unsigned_integer (op_ptr, op_end, 4, byte_order, &uoffset); kind_u.param_cu_off = (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_end, int len, bfd_endian byte_order, T &r) { gdb_assert (len >= 0); gdb_assert (len <= sizeof (T)); const gdb_byte *data = buf; buf = safe_skip_bytes (buf, buf_end, len); r = 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 = safe_read_unsigned_integer (op_ptr, op_end, 4, byte_order, &kind_u.param_cu_off); > +{ > + gdb_assert (len >= 0); I would add an assert at len is <= the size of the destination type, as shown above in my example. > + const gdb_byte *data = buf; > + buf = safe_skip_bytes (buf, buf_end, len); > + *r = 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 >= 0); > + > + const gdb_byte *data = buf; > + buf = safe_skip_bytes (buf, buf_end, len); > + *r = 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 == NULL) > return -1; > if ((int) dwarf_reg != 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 != 0) > return -1; > + if (buf >= 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 == buf_end`, but not `buf > buf_end`. If so, I would do: gdb_assert (buf <= buf_end); if (buf == 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 == DW_OP_deref) > { > @@ -1498,7 +1538,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view block, > { > buf++; > if (buf >= 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 = (op == DW_OP_deref ? this->m_addr_size : *op_ptr++); > + int addr_size = this->m_addr_size; > + if (op != DW_OP_deref) > + { > + op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1, > + byte_order, &uoffset); > + addr_size = uoffset; > + } Please write it with the two branches like this: int addr_size; if (op == DW_OP_deref) addr_size = = this->m_addr_size; else { op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1, byte_order, &uoffset); addr_size = uoffset; } Add an empty line after the curly brace that closes the scope. Simon