From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) by sourceware.org (Postfix) with ESMTPS id 3C370385741C for ; Thu, 14 Apr 2022 19:26:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3C370385741C 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-f51.google.com with SMTP id b19so8219782wrh.11 for ; Thu, 14 Apr 2022 12:26:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:from:to:references:in-reply-to :content-transfer-encoding; bh=UlrKXIbQBbeBSJd47uHwhf+M39NsaQtxdiE2E9I621I=; b=K9CUixFKGpbkMXpk7adtHV+3uwV6ElDt/aK3+uajIfLea5GDO0cz7y1EqIG7PfHO3g H3iD3uM1lO1/USS+uEpMQeIOnBvWddDeH+eyFtUIWCEv8GC/0p6G4b8cHgpUHGL4ZxSL bx7DJtCVBVOpe03g81i0qkox5DJ3lmHhrlSZ6muM6IZ+9WIM09yOuQhcevUDDPDXOjqC ++Ud8TYGBiFoi+1sjidornavTWBl6PjEe3EItDn5bBIE+7b8Sn4t22HdQFx6lvBTh4yQ stI7dBm5xbxP5RvBRSj5IrEPujeeebpQ3BD6j29zV5Cz7J8aDjRDpqieyeIwzOh44P4M YB7w== X-Gm-Message-State: AOAM532io9+EzRD5JhLWkiatDLhYf1MOS6Q8gpsoe7Nd9YZ38kyiXPL2 ws3rldOmO8RUpGElKxmd6uF3tzN9RN8= X-Google-Smtp-Source: ABdhPJxXts10vy00YveXdFXeTpJJstR51gQkHB0EqBPIhJCjNpZntHdprTmKtnEDjkOvvgwEwM5FlQ== X-Received: by 2002:adf:82c1:0:b0:207:9d8b:dd6c with SMTP id 59-20020adf82c1000000b002079d8bdd6cmr3090612wrc.4.1649964363563; Thu, 14 Apr 2022 12:26:03 -0700 (PDT) Received: from ?IPV6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id b1-20020a5d40c1000000b00207b49d3023sm2369319wrq.44.2022.04.14.12.26.02 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 14 Apr 2022 12:26:02 -0700 (PDT) Message-ID: Date: Thu, 14 Apr 2022 20:26:01 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH v2 2/4] gdbserver: special case target_write_memory len==0 Content-Language: en-US From: Pedro Alves To: gdb-patches@sourceware.org References: <20220404143750.758138-1-pedro@palves.net> <20220404143750.758138-3-pedro@palves.net> In-Reply-To: <20220404143750.758138-3-pedro@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_NUMSUBJECT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Apr 2022 19:26:07 -0000 FYI, I've merged this series, with one small detail changed: > diff --git a/gdbserver/target.cc b/gdbserver/target.cc > index 5009146d663..a809ff43984 100644 > --- a/gdbserver/target.cc > +++ b/gdbserver/target.cc > @@ -152,6 +152,13 @@ int > target_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, > ssize_t len) > { > + /* GDB may send X packets with LEN==0, for probing packet support. > + If we let such a request go through, then buffer.data() below may > + return NULL, which may confuse target implementations. Handle it > + here to avoid lower levels having to care about this case. */ > + if (len == 0) > + return 0; > + > /* Make a copy of the data because check_mem_write may need to > update it. */ > gdb::byte_vector buffer (myaddr, myaddr + len); Lancelot suggested off list to do the same on the read end, in case GDB ever changes, or we connect to a different client. It makes sense and doesn't hurt so I've done that in the merged patch, as below. >From 366e3746c572c2c78454761e62fa9181cba413ca Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 31 Mar 2022 22:04:42 +0100 Subject: [PATCH] gdbserver: special case target_write_memory len==0 The next patch in this series adds a common helper routine for both memory reads and writes, like this: static int proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf, const gdb_byte *writebuf, int len) { gdb_assert ((readbuf == nullptr) != (writebuf == nullptr)); ... } int linux_process_target::read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len) { return proc_xfer_memory (memaddr, myaddr, nullptr, len); } linux_process_target::write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len) { return proc_xfer_memory (memaddr, nullptr, myaddr, len); } Surprisingly, the assertion fails. That happens because it can happen that target_write_memory is called with LEN==0, due to this in gdb/remote.c: /* Determine whether the remote target supports binary downloading. This is accomplished by sending a no-op memory write of zero length to the target at the specified address. (...) */ void remote_target::check_binary_download (CORE_ADDR addr) { ... p = rs->buf.data (); *p++ = 'X'; p += hexnumstr (p, (ULONGEST) addr); *p++ = ','; p += hexnumstr (p, (ULONGEST) 0); *p++ = ':'; *p = '\0'; In this scenario, in gdbserver's target_write_memory, the "myaddr" argument of the_target->write_memory is passed the data() of a local gdb::byte_vector (which is a specialized std::vector). It's valid for std::vector::data() to return NULL when the vector is empty. This commit adds an early return to target_write_memory to avoid target backends having to care about this. For good measure, do the same on the read side, in read_inferior_memory. Change-Id: Iac8f04fcf99014c624ef4036bd318ca1771ad491 --- gdbserver/target.cc | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/gdbserver/target.cc b/gdbserver/target.cc index 5009146d663..e9d1e1aa38c 100644 --- a/gdbserver/target.cc +++ b/gdbserver/target.cc @@ -124,8 +124,14 @@ done_accessing_memory (void) int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len) { - int res; - res = the_target->read_memory (memaddr, myaddr, len); + /* At the time of writing, GDB only sends write packets with LEN==0, + not read packets (see comment in target_write_memory), but it + doesn't hurt to prevent problems if it ever does, or we're + connected to some client other than GDB that does. */ + if (len == 0) + return 0; + + int res = the_target->read_memory (memaddr, myaddr, len); check_mem_read (memaddr, myaddr, len); return res; } @@ -152,6 +158,13 @@ int target_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, ssize_t len) { + /* GDB may send X packets with LEN==0, for probing packet support. + If we let such a request go through, then buffer.data() below may + return NULL, which may confuse target implementations. Handle it + here to avoid lower levels having to care about this case. */ + if (len == 0) + return 0; + /* Make a copy of the data because check_mem_write may need to update it. */ gdb::byte_vector buffer (myaddr, myaddr + len); base-commit: 1a7c41d5ece7d0d1aa77d8019ee46f03181854fa prerequisite-patch-id: 60dfa0d6969565f53d13fb4e0db16da0152b347a -- 2.26.2