From: Tom Tromey Date: Wed, 11 Mar 2026 20:17:50 +0000 (-0600) Subject: Do not write negative PID or TID in remote protocol X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=c2d4b5e1135f75f7e4ef7ff68fa899bfe483671b;p=thirdparty%2Fbinutils-gdb.git Do not write negative PID or TID in remote protocol Currently both gdb and gdbserver can write a negative number for the PID or TID. However, the only negative value that really makes sense is the special case of "-1" -- in other cases if the PID or TID has the high bit set, it should still be written as a positive number. This patch attempts to fix the bug. v2 of this patch combines the implementations and moves them to gdbsupport. I tried making these standalone functions in rsp-low.cc, but that runs afoul of libipa. So, I made them methods of ptid_t. The new unit tests pointed out that round-tripping a sign-extended number didn't really work, because the checks were done via ULONGEST. It's kind of unfortunate that the PID and LWP are host-dependent types. This should probably be fixed, but I haven't done so here. Meanwhile I changed the checks to use the corresponding unsigned type. v1 is here https://inbox.sourceware.org/gdb-patches/20260311202152.2704410-1-tromey@adacore.com/ Regression tested on x86-64 Fedora 43. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25111 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33979 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33983 --- diff --git a/gdb/remote.c b/gdb/remote.c index f8c7383cb03..33222346d06 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -3684,31 +3684,17 @@ static char *pack_threadlist_request (char *pkt, int startflag, static int remote_newthread_step (threadref *ref, void *context); -/* Write a PTID to BUF. ENDBUF points to one-passed-the-end of the +/* Write a PTID to BUF. ENDBUF points to one-past-the-end of the buffer we're allowed to write to. Returns BUF+CHARACTERS_WRITTEN. */ char * remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid) { - ptid_t::pid_type pid; - ptid_t::lwp_type lwp; - - if (m_features.remote_multi_process_p ()) - { - pid = ptid.pid (); - if (pid < 0) - buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid); - else - buf += xsnprintf (buf, endbuf - buf, "p%x.", pid); - } - lwp = ptid.lwp (); - if (lwp < 0) - buf += xsnprintf (buf, endbuf - buf, "-%lx", -lwp); - else - buf += xsnprintf (buf, endbuf - buf, "%lx", lwp); - - return buf; + std::string repr = ptid.to_rsp_string (m_features.remote_multi_process_p ()); + gdb_assert (repr.length () < endbuf - buf); + strcpy (buf, repr.c_str ()); + return buf + repr.length (); } /* Extract a PTID from BUF. If non-null, OBUF is set to one past the @@ -3718,67 +3704,21 @@ remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid) static ptid_t read_ptid (const char *buf, const char **obuf) { - const char *p = buf; - const char *pp; - ptid_t::pid_type pid = 0; - ptid_t::lwp_type lwp = 0; - ULONGEST hex; - - if (*p == 'p') - { - /* Multi-process ptid. */ - pp = unpack_varlen_hex (p + 1, &hex); - if ((pp == (p + 1)) || (*pp != '.')) - error (_("invalid remote ptid: %s"), buf); - - pid = (ptid_t::pid_type) (LONGEST) hex; - if (hex != ((ULONGEST) pid)) - error (_("invalid remote ptid: %s"), buf); - - p = pp + 1; - pp = unpack_varlen_hex (p, &hex); - if (pp == p) - error (_("invalid remote ptid: %s"), buf); - - lwp = (ptid_t::lwp_type) (LONGEST) hex; - if (hex != ((ULONGEST) lwp)) - error (_("invalid remote ptid: %s"), buf); - - if (obuf) - *obuf = pp; - - return ptid_t (pid, lwp); - } - - /* No multi-process. Just a thread id. */ - pp = unpack_varlen_hex (p, &hex); - - /* Return null_ptid when no thread id is found. */ - if (p == pp) - { - if (obuf) - *obuf = pp; - return null_ptid; - } - - lwp = (ptid_t::lwp_type) (LONGEST) hex; - if (hex != ((ULONGEST) lwp)) - error (_("invalid remote ptid: %s"), buf); - - /* Since the stub is not sending a process id, default to what's - current_inferior, unless it doesn't have a PID yet. If so, - then since there's no way to know the pid of the reported - threads, use the magic number. */ - inferior *inf = current_inferior (); - if (inf->pid == 0) - pid = magic_null_ptid.pid (); - else - pid = inf->pid; - - if (obuf) - *obuf = pp; - - return ptid_t (pid, lwp); + return ptid_t::parse (buf, obuf, false, + [] () + { + /* Since the stub is not sending a process id, default to + what's current_inferior, unless it doesn't have a PID yet. + If so, then since there's no way to know the pid of the + reported threads, use the magic number. */ + inferior *inf = current_inferior (); + ptid_t::pid_type pid; + if (inf->pid == 0) + pid = magic_null_ptid.pid (); + else + pid = inf->pid; + return pid; + }); } static int diff --git a/gdb/unittests/ptid-selftests.c b/gdb/unittests/ptid-selftests.c index 7b8ada9500a..c02401db952 100644 --- a/gdb/unittests/ptid-selftests.c +++ b/gdb/unittests/ptid-selftests.c @@ -18,6 +18,7 @@ along with this program. If not, see . */ #include "gdbsupport/ptid.h" +#include "gdbsupport/selftest.h" #include namespace selftests { @@ -149,6 +150,51 @@ static_assert (both.matches (both), "both matches both"); static_assert (!ptid_t (2, 2, 2).matches (both), "other both doesn't match both"); +/* Helper function to parse and return a single PTID. */ +static ptid_t +parse_one (const char *str, bool for_remote) +{ + const char *out; + ptid_t result = ptid_t::parse (str, &out, for_remote, + [] () + { + return ptid_t::pid_type (23); + }); + SELF_CHECK (*out == '\0'); + return result; +} + +/* Test ptid_t reading and writing. */ +static void +test () +{ + SELF_CHECK (minus_one_ptid.to_rsp_string (false) == "0"); + SELF_CHECK (minus_one_ptid.to_rsp_string (true) == "p-1.0"); + + SELF_CHECK (lwp.to_rsp_string (false) == "2"); + SELF_CHECK (lwp.to_rsp_string (true) == "p1.2"); + + ptid_t negative (-57, -32); + /* This is kind of lame but currently the types are host-dependent + so we have to adapt to those here. */ + std::string the_pid = string_printf ("%x", (unsigned) -57); + std::string the_lwp = string_printf ("%lx", (unsigned long) -32); + std::string full = "p" + the_pid + "." + the_lwp; + SELF_CHECK (negative.to_rsp_string (false) == the_lwp); + SELF_CHECK (negative.to_rsp_string (true) == full); + + SELF_CHECK (parse_one ("p1.2", false) == lwp); + SELF_CHECK (parse_one ("p1.-1", true) == ptid_t (1, -1)); + SELF_CHECK (parse_one ("-1", true) == minus_one_ptid); + SELF_CHECK (parse_one ("0", false) == null_ptid); + SELF_CHECK (parse_one (full.c_str (), false) == negative); + SELF_CHECK (parse_one (full.c_str (), true) == negative); +} } /* namespace ptid */ } /* namespace selftests */ + +INIT_GDB_FILE (ptid_selftests) +{ + selftests::register_test ("ptid_t", selftests::ptid::test); +} diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc index e99f34dfc6d..ffde4956776 100644 --- a/gdbserver/remote-utils.cc +++ b/gdbserver/remote-utils.cc @@ -519,43 +519,9 @@ char * write_ptid (char *buf, ptid_t ptid) { client_state &cs = get_client_state (); - ptid_t::pid_type pid; - ptid_t::lwp_type lwp; - - if (cs.multi_process) - { - pid = ptid.pid (); - if (pid < 0) - buf += sprintf (buf, "p-%x.", -pid); - else - buf += sprintf (buf, "p%x.", pid); - } - lwp = ptid.lwp (); - if (lwp < 0) - buf += sprintf (buf, "-%lx", -lwp); - else - buf += sprintf (buf, "%lx", lwp); - - return buf; -} - -static ULONGEST -hex_or_minus_one (const char *buf, const char **obuf) -{ - ULONGEST ret; - - if (startswith (buf, "-1")) - { - ret = (ULONGEST) -1; - buf += 2; - } - else - buf = unpack_varlen_hex (buf, &ret); - - if (obuf) - *obuf = buf; - - return ret; + std::string repr = ptid.to_rsp_string (cs.multi_process); + strcpy (buf, repr.c_str ()); + return buf + repr.length (); } /* Extract a PTID from BUF. If non-null, OBUF is set to one past the last @@ -563,61 +529,14 @@ hex_or_minus_one (const char *buf, const char **obuf) ptid_t read_ptid (const char *buf, const char **obuf) { - const char *p = buf; - const char *pp; - ptid_t::pid_type pid = 0; - ptid_t::lwp_type lwp = 0; - ULONGEST hex; - - if (*p == 'p') - { - /* Multi-process ptid. */ - pp = unpack_varlen_hex (p + 1, &hex); - if (pp == (p + 1) || *pp != '.') - error (_("invalid remote ptid: %s\n"), buf); - - pid = (ptid_t::pid_type) (LONGEST) hex; - if (hex != ((ULONGEST) pid)) - error (_("invalid remote ptid: %s"), buf); - - p = pp + 1; - hex = hex_or_minus_one (p, &pp); - if (pp == p) - error (_("invalid remote ptid: %s\n"), buf); - - lwp = (ptid_t::lwp_type) (LONGEST) hex; - if (hex != ((ULONGEST) lwp)) - error (_("invalid remote ptid: %s"), buf); - - if (obuf) - *obuf = pp; - - return ptid_t (pid, lwp); - } - - /* No multi-process. Just a thread id. */ - hex = hex_or_minus_one (p, &pp); - - /* Handle special thread ids. */ - if (hex == (ULONGEST) -1) - return minus_one_ptid; - - if (hex == 0) - return null_ptid; - - lwp = (ptid_t::lwp_type) (LONGEST) hex; - if (hex != ((ULONGEST) lwp)) - error (_("invalid remote ptid: %s"), buf); - - /* Since GDB is not sending a process id (multi-process extensions - are off), then there's only one process. Default to the first in - the list. */ - pid = get_first_process ()->pid; - - if (obuf) - *obuf = pp; - - return ptid_t (pid, lwp); + return ptid_t::parse (buf, obuf, true, + [] () + { + /* Since GDB is not sending a process id (multi-process + extensions are off), then there's only one process. + Default to the first in the list. */ + return get_first_process ()->pid; + }); } /* Write COUNT bytes in BUF to the client. Returns true if all bytes diff --git a/gdbsupport/ptid.cc b/gdbsupport/ptid.cc index 78978c15c06..731a5bc3da3 100644 --- a/gdbsupport/ptid.cc +++ b/gdbsupport/ptid.cc @@ -19,6 +19,7 @@ #include "ptid.h" #include "print-utils.h" +#include "rsp-low.h" /* See ptid.h for these. */ @@ -27,8 +28,114 @@ ptid_t const minus_one_ptid = ptid_t::make_minus_one (); /* See ptid.h. */ +std::string +ptid_t::to_rsp_string (bool multi) const +{ + char result[50]; + char *buf = result; + char *endbuf = &result[sizeof (result)]; + + if (multi) + { + if (m_pid == -1) + buf += xsnprintf (buf, endbuf - buf, "p-1."); + else + buf += xsnprintf (buf, endbuf - buf, "p%x.", (unsigned) m_pid); + } + if (m_lwp == -1) + xsnprintf (buf, endbuf - buf, "-1"); + else + xsnprintf (buf, endbuf - buf, "%lx", (unsigned long) m_lwp); + + return result; +} + +/* See ptid.h. */ + std::string ptid_t::to_string () const { return string_printf ("%d.%ld.%s", m_pid, m_lwp, pulongest (m_tid)); } + +/* See ptid.h. */ + +static ULONGEST +hex_or_minus_one (const char *buf, const char **obuf, bool for_remote) +{ + ULONGEST ret; + + if (for_remote && startswith (buf, "-1")) + { + ret = (ULONGEST) -1; + buf += 2; + } + else + buf = unpack_varlen_hex (buf, &ret); + + if (obuf != nullptr) + *obuf = buf; + + return ret; +} + +ptid_t +ptid_t::parse (const char *buf, const char **obuf, bool for_remote, + gdb::function_view default_pid) +{ + const char *p = buf; + const char *pp; + ptid_t::pid_type pid = 0; + ptid_t::lwp_type lwp = 0; + ULONGEST hex; + + using unsigned_pid_type = std::make_unsigned::type; + using unsigned_lwp_type = std::make_unsigned::type; + + if (*p == 'p') + { + /* Multi-process ptid. */ + pp = unpack_varlen_hex (p + 1, &hex); + if (pp == (p + 1) || *pp != '.') + error (_("invalid remote ptid: %s"), buf); + + pid = (ptid_t::pid_type) hex; + if (hex != ((unsigned_pid_type) pid)) + error (_("invalid remote ptid: %s"), buf); + + p = pp + 1; + hex = hex_or_minus_one (p, &pp, for_remote); + if (pp == p) + error (_("invalid remote ptid: %s"), buf); + + lwp = (ptid_t::lwp_type) hex; + if (hex != ((unsigned_lwp_type) lwp)) + error (_("invalid remote ptid: %s"), buf); + + if (obuf != nullptr) + *obuf = pp; + + return ptid_t (pid, lwp); + } + + /* No multi-process. Just a thread id. */ + hex = hex_or_minus_one (p, &pp, for_remote); + + /* Handle special thread ids. */ + if (hex == (ULONGEST) -1) + return minus_one_ptid; + + if (hex == 0) + return null_ptid; + + lwp = (ptid_t::lwp_type) hex; + if (hex != ((unsigned_lwp_type) lwp)) + error (_("invalid remote ptid: %s"), buf); + + pid = default_pid (); + + if (obuf != nullptr) + *obuf = pp; + + return ptid_t (pid, lwp); +} diff --git a/gdbsupport/ptid.h b/gdbsupport/ptid.h index dfc0fd77df0..ef6da77eb7e 100644 --- a/gdbsupport/ptid.h +++ b/gdbsupport/ptid.h @@ -35,6 +35,7 @@ #include #include #include "gdbsupport/common-types.h" +#include "gdbsupport/function-view.h" class ptid_t { @@ -130,6 +131,12 @@ public: || *this == filter); } + /* Return a remote-protocol-compatible string representation of this + ptid. The MULTI argument controls whether the multi-process + representation is used. */ + + std::string to_rsp_string (bool multi) const; + /* Return a string representation of the ptid. This is only meant to be used in debug messages. */ @@ -146,6 +153,16 @@ public: static constexpr ptid_t make_minus_one () { return ptid_t (-1, 0, 0); } + /* Extract a PTID from BUF. If non-null, OBUF is set to one past + the last parsed char. FOR_REMOTE is true for "gdbserver" style + parsing, that is, applying a special meaning to "-1" (see RSP + docs for details). DEFAULT_PID is called when the parsed string + does not provide a process ID; it should return the default PID + to use. This function throws on error. */ + + static ptid_t parse (const char *buf, const char **obuf, bool for_remote, + gdb::function_view default_pid); + private: /* Process id. */ pid_type m_pid;