From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id 4C52A3848BA6 for ; Fri, 7 Jul 2023 17:27:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4C52A3848BA6 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-x42c.google.com with SMTP id ffacd0b85a97d-3142860734aso2946404f8f.1 for ; Fri, 07 Jul 2023 10:27:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688750794; x=1691342794; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=29LxGhzhijlQkJtTPhjKRcqI+h2tUMdYj9V6ly2APXE=; b=AdFAD9Kh6g/BNVwOfZczLiekkB4Mn7xwvA4sIEHZvrWkzSdroOZJm+L8Y96YdQEdJU MCerX6noazxXhmsdKM/FtO2sygfuWL3gyk6y9kdJIk8SQBr7EGa1pJkE5s8vI3eX4Gbd IFcSeJ1q+hKbRFqZ+SYUcWilp3vH6YzV78nwNloKIfrcdR1CAJLRkfEFP3bug0aS6k+D kVTpFSFviRD7YC4QLNmo0+36TCL6sM/mXDS6V6EiGGAIDa82X3d/ZKqMXBhZEjPep3YQ xTAduuiVlP/T2JRJj3f8ef3EyU10h1qidOC10xaGqcbv7DGmVwwI4IS84jJ07hYgrqEy T0BQ== X-Gm-Message-State: ABy/qLYHOPkBJVoSaa/KUSy8bEX3JWIyDz6/dzgsPcut2e134FGIHsmJ jNBGIvuWwY5DJ4mvC7q5DX6UBCA8XTM= X-Google-Smtp-Source: APBJJlHDenII84UAT9L79wFvs3HDeCTJcLg+TuohULLYQv1xHiJ24/lLBcxPdJtQ+5WOEVpTa52dcg== X-Received: by 2002:adf:ee8f:0:b0:313:f2bf:4c3c with SMTP id b15-20020adfee8f000000b00313f2bf4c3cmr7687034wro.24.1688750794304; Fri, 07 Jul 2023 10:26:34 -0700 (PDT) Received: from ?IPV6:2001:8a0:f91d:bc00:1b6e:208c:60cc:dcc? ([2001:8a0:f91d:bc00:1b6e:208c:60cc:dcc]) by smtp.gmail.com with ESMTPSA id s16-20020a5d4ed0000000b0031411e46af3sm4919657wrv.97.2023.07.07.10.26.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 07 Jul 2023 10:26:33 -0700 (PDT) Message-ID: <2d33fb4e-380a-310c-d4b4-cf3dfe69eaec@palves.net> Date: Fri, 7 Jul 2023 18:26:32 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCHv5 03/11] gdbserver: allow agent expressions to fail with invalid memory access Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: <0ef54ed920e1262a07f4a061ecc08bea8fcaee23.1678987897.git.aburgess@redhat.com> <01059f8a-0e59-55b5-f530-190c26df5ba3@palves.net> <87pm53k7om.fsf@redhat.com> From: Pedro Alves In-Reply-To: <87pm53k7om.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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: 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. Pedro Alves > > Thanks, > Andrew > > --- > > commit 35e78a7699618f0bb1c111cdc54d9f9bcb0171c9 > 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 figured that, > if I convert eval_result_names into a function containing a switch > then I can make use of DIAGNOSTIC_ERROR_SWITCH to have the compiler > ensure that we always cover every possible error code. > > I know (from the comments) that tracepoint.cc is built both into > gdbserver, and into a shared library that the inferior might load (for > fast tracepoint support), so I did consider whether this change could > break the API of that shared library. However, eval_result_names is a > static array, so I don't believe that this is part of the external > API, as such, converting this array into a static function shouldn't > impact any users. > > [1] https://inbox.sourceware.org/gdb-patches/01059f8a-0e59-55b5-f530-190c26df5ba3@palves.net/ > > diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc > index 1f31e0cc05f..879ad734b66 100644 > --- a/gdbserver/tracepoint.cc > +++ b/gdbserver/tracepoint.cc > @@ -857,17 +857,43 @@ EXTERN_C_POP > > 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" > - }; > +/* Return a string representing the error in ERR. This should not be > + called when ERR is expr_eval_no_error. */ > + > +static const char * const > +eval_result_error_name (enum eval_result_type err) > +{ > + gdb_assert (err != expr_eval_no_error); > + > +DIAGNOSTIC_PUSH > +DIAGNOSTIC_ERROR_SWITCH > + switch (err) > + { > + case expr_eval_no_error: > + break; > + case expr_eval_empty_expression: > + return "terror:empty expression"; > + case expr_eval_empty_stack: > + return "terror:empty stack"; > + case expr_eval_stack_overflow: > + return "terror:stack overflow"; > + case expr_eval_stack_underflow: > + return "terror:stack underflow"; > + case expr_eval_unhandled_opcode: > + return "terror:unhandled opcode"; > + case expr_eval_unrecognized_opcode: > + return "terror:unrecognized opcode"; > + case expr_eval_divide_by_zero: > + return "terror:divide by zero"; > + case expr_eval_invalid_goto: > + return "terror:invalid goto"; > + case expr_eval_invalid_memory_access: > + return "terror:invalid memory access"; > + } > +DIAGNOSTIC_POP > + > + gdb_assert_not_reached ("invalid error type"); > +} > > #endif > > @@ -3447,7 +3473,8 @@ stop_tracing (void) > else if (expr_eval_result != expr_eval_no_error) > { > trace_debug ("Stopping the trace because of an expression eval error"); > - tracing_stop_reason = eval_result_names[expr_eval_result]; > + tracing_stop_reason > + = eval_result_error_name ((enum eval_result_type) expr_eval_result); > tracing_stop_tpnum = error_tracepoint->number; > } > #ifndef IN_PROCESS_AGENT >