From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 436BD3857704 for ; Mon, 10 Jul 2023 10:32:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 436BD3857704 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688985158; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=UdDuQHRtKrsNwSitUIAa2gyv734xv00QXJDwRHOsycU=; b=JQXaRU0RdYJsEjNbetFUw3Z1DLBiTOU7q5sOvJhQhRR2TaYacNRbbSIv1bm3STnbhi+X64 W3+bi1VjHwTAtkRsu34DDfDF8XVmq0RyuTd1J9EYRnyCyVKQCDsSwT3jSxk4G7DpXtNg20 TfsfrSJ+hQ+Uf4Qig7Mp0V6axoyazaU= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-155-EtIPKKSfMbKB2pAu5Vb4eg-1; Mon, 10 Jul 2023 06:32:37 -0400 X-MC-Unique: EtIPKKSfMbKB2pAu5Vb4eg-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-3f5df65f9f4so26551165e9.2 for ; Mon, 10 Jul 2023 03:32:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688985156; x=1691577156; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=UdDuQHRtKrsNwSitUIAa2gyv734xv00QXJDwRHOsycU=; b=TmAzTihKdr4lYrCNG50/QDkE+iXyCj5JioXp6TEn7qUcamSI7uHzyjHZn6P8UXdRux zIkHtLDE/R+mPoUATLrPHru7p1k+8JYAoLtu/IzrrpKs8tPY1IhoGTo3kdsNMmziUpNf ykIZs3YGhp68D6ptB8qnOax4lpN6+5s+ujSpU6ghDj17Rwd68f76pu/SbTRz/fcQSkoe 9uLoJRCKFrnhkqYhyszNv1Ke0UKtOtxOZfM6ArmG0hAlDFyS0fYFJ2ggo00vtISWeUT0 VzYPKwBt0hcwp2PuLX7KLUxRPyZAGWT3j1+ybUqSltI9uBDJKH8QgWcrx9Tl/laWlCD3 GmHA== X-Gm-Message-State: ABy/qLYZTM2UpHXeyhjkxzECBWvN5BeY/Oh0aOBW4iC8WmaWDN2vxS4D zPO6cbmp5kcGXWQkmo2exzv5A94Vo/mlKXoTM3Di9K7WzslQpIljMRPLZgV+I9cmZ6XQCrd+Owi SThZtUQoZgKJlokaN+x5lfQ== X-Received: by 2002:a05:600c:c8:b0:3fb:b05d:f27c with SMTP id u8-20020a05600c00c800b003fbb05df27cmr10229694wmm.34.1688985156485; Mon, 10 Jul 2023 03:32:36 -0700 (PDT) X-Google-Smtp-Source: APBJJlErNpn53tLeuno5qH9JkxoRP+9xANEw46qYSMRrtEUZ88h2j3K/Om90NxYM8+QzgieuKAt5Tw== X-Received: by 2002:a05:600c:c8:b0:3fb:b05d:f27c with SMTP id u8-20020a05600c00c800b003fbb05df27cmr10229672wmm.34.1688985155962; Mon, 10 Jul 2023 03:32:35 -0700 (PDT) Received: from localhost (2.72.115.87.dyn.plus.net. [87.115.72.2]) by smtp.gmail.com with ESMTPSA id y23-20020a1c4b17000000b003fbd0c50ba2sm9897980wma.32.2023.07.10.03.32.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Jul 2023 03:32:35 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCHv5 03/11] gdbserver: allow agent expressions to fail with invalid memory access In-Reply-To: <2d33fb4e-380a-310c-d4b4-cf3dfe69eaec@palves.net> References: <0ef54ed920e1262a07f4a061ecc08bea8fcaee23.1678987897.git.aburgess@redhat.com> <01059f8a-0e59-55b5-f530-190c26df5ba3@palves.net> <87pm53k7om.fsf@redhat.com> <2d33fb4e-380a-310c-d4b4-cf3dfe69eaec@palves.net> Date: Mon, 10 Jul 2023 11:32:34 +0100 Message-ID: <87edlgjbul.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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: Pedro Alves writes: > On 2023-07-07 17:28, Andrew Burgess wrote: >> Pedro Alves writes: >> >>> On 2023-03-16 17:36, Andrew Burgess via Gdb-patches wrote: >>>> This commit extends gdbserver to take account of a failed memory >>>> access from agent_mem_read, and to return a new eval_result_type >>>> expr_eval_invalid_memory_access. >>>> >>>> I have only updated the agent_mem_read calls related directly to >>>> reading memory, I have not updated any of the calls related to >>>> tracepoint data collection. This is just because I'm not familiar >>>> with that area of gdb/gdbserver, and I don't want to break anything, >>>> so leaving the existing behaviour untouched seems like the safest >>>> approach. >>> >>> I think this should update the gdbserver/tracepoint.cc:eval_result_names array at least, >>> though. Otherwise I think a memory error during collection is going to trigger undefined >>> behavior, crash if we're lucky. >> >> Good spot. >> >> What are your thoughts on the patch below that should address this >> issue? > > Hmm. If we're changing how the array is built completely, you could go a step > further and convert to a .def file (like e.g., gdb/unwind_stop_reasons.def) that is > included by both ax.h and tracepoint.cc. That would keep the enum names and their > string representations in the same place. It would also dispense with the > diagnostics pragmas, as the problem scenario goes away by design. Thanks for the suggestion. Here's an attempt to switch to a .def file. Thoughts? Thanks, Andrew --- commit 75a86840a474097a19f4d04c9232981df3bf8066 Author: Andrew Burgess Date: Fri Jul 7 17:18:46 2023 +0100 gdbserver: handle all eval_result_type values in tracepoint.cc It was pointed out[1] that after this commit: commit 3812b38d8de5804ad3eadd6c7a5d532402ddabab Date: Thu Oct 20 11:14:33 2022 +0100 gdbserver: allow agent expressions to fail with invalid memory access Now that agent expressions might fail with the error expr_eval_invalid_memory_access, we might overflow the eval_result_names array in tracepoint.cc. This is because the eval_result_names array does not include a string for either expr_eval_invalid_goto or expr_eval_invalid_memory_access. I don't know if having expr_eval_invalid_goto missing is also a problem, but it feels like eval_result_names should just include a string for every possible error. I could just add two more strings into the array, but I figure that a more robust solution will be to move all of the error types, and their associated strings, into a new ax-result-types.def file, and to then include this file in both ax.h and tracepoint.cc in order to build the enum eval_result_type and the eval_result_names string array. Doing this means it is impossible to have a missing error string in the future. [1] https://inbox.sourceware.org/gdb-patches/01059f8a-0e59-55b5-f530-190c26df5ba3@palves.net/ diff --git a/gdbserver/ax-result-types.def b/gdbserver/ax-result-types.def new file mode 100644 index 00000000000..3a832c44be2 --- /dev/null +++ b/gdbserver/ax-result-types.def @@ -0,0 +1,44 @@ +/* Agent expression result types. + + Copyright (C) 2023 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +/* The AX_RESULT_TYPE macro is used to define a result type that can be + generated by agent expression evaluation. The first macro argument is + tye name of an enum entry, and the second is a string that describes + this result type. */ + +AX_RESULT_TYPE (expr_eval_no_error, + "terror:no error") +AX_RESULT_TYPE (expr_eval_empty_expression, + "terror:empty expression") +AX_RESULT_TYPE (expr_eval_empty_stack, + "terror:empty stack") +AX_RESULT_TYPE (expr_eval_stack_overflow, + "terror:stack overflow") +AX_RESULT_TYPE (expr_eval_stack_underflow, + "terror:stack underflow") +AX_RESULT_TYPE (expr_eval_unhandled_opcode, + "terror:unhandled opcode") +AX_RESULT_TYPE (expr_eval_unrecognized_opcode, + "terror:unrecognized opcode") +AX_RESULT_TYPE (expr_eval_divide_by_zero, + "terror:divide by zero") +AX_RESULT_TYPE (expr_eval_invalid_goto, + "terror:invalid goto") +AX_RESULT_TYPE (expr_eval_invalid_memory_access, + "terror:invalid memory access") diff --git a/gdbserver/ax.h b/gdbserver/ax.h index c98e36a83c6..60e307ca42d 100644 --- a/gdbserver/ax.h +++ b/gdbserver/ax.h @@ -33,16 +33,9 @@ struct traceframe; enum eval_result_type { - expr_eval_no_error, - expr_eval_empty_expression, - expr_eval_empty_stack, - expr_eval_stack_overflow, - expr_eval_stack_underflow, - expr_eval_unhandled_opcode, - expr_eval_unrecognized_opcode, - expr_eval_divide_by_zero, - expr_eval_invalid_goto, - expr_eval_invalid_memory_access +#define AX_RESULT_TYPE(ENUM,STR) ENUM, +#include "ax-result-types.def" +#undef AX_RESULT_TYPE }; struct agent_expr diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc index 1f31e0cc05f..609d49a87ef 100644 --- a/gdbserver/tracepoint.cc +++ b/gdbserver/tracepoint.cc @@ -859,14 +859,9 @@ static struct tracepoint *last_tracepoint; static const char * const eval_result_names[] = { - "terror:in the attic", /* this should never be reported */ - "terror:empty expression", - "terror:empty stack", - "terror:stack overflow", - "terror:stack underflow", - "terror:unhandled opcode", - "terror:unrecognized opcode", - "terror:divide by zero" +#define AX_RESULT_TYPE(ENUM,STR) STR, +#include "ax-result-types.def" +#undef AX_RESULT_TYPE }; #endif