RE: [PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Tom,

>In general, read_ptid() and write_ptid() don't seem to care about
>types and conversions; read_ptid(), for example, reads everything
>into ULONGEST and then constructs a ptid without checking for
>overflows.
>
>I could add a fix to this series, but, as with the other patches, this
>would be solely based on reviewing the code without an actual bug
>(I cannot test this zephyr target) nor a test.

Here's the patch.  I'll add it to v2 of this series.

I tested this on ubuntu 22.04 running on x86-64.  Is it just me or is
everybody seeing lots of sporadic fails that appear and disappear
between runs?

I'm running tests with FORCE_PARALLEL=1 for each patch in a series
plus a few upstream patches in front and then filter out fails that
appear and disappear manually.

Regards,
Markus.

---

commit b6ec4c40ec46fdba710369e2541699d2d704c828
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Wed Sep 24 12:08:55 2025 +0000

    gdb, gdbserver: fix read/write_ptid types
    
    In write_ptid(), a ptid's LWP member, which is declared long, is stored in
    an int local variable before printing, potentially truncating it.  Fix it.
    
    In read_ptid(), both PID and LWP are read as ULONGEST and then cast to
    their respective type without checking for overflows.  Fix it.
    
    In read_ptid(), an empty component is treated as zero.  Diagnose that as
    an error, instead.
    
    CC: Tom Tromey <tom@tromey.com>

diff --git a/gdb/remote.c b/gdb/remote.c
index 961322f398b..6d609ced02b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3464,7 +3464,8 @@ static int remote_newthread_step (threadref *ref, void *context);
 char *
 remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid)
 {
-  int pid, tid;
+  ptid_t::pid_type pid;
+  ptid_t::lwp_type lwp;
 
   if (m_features.remote_multi_process_p ())
     {
@@ -3474,11 +3475,11 @@ remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid)
       else
 	buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
     }
-  tid = ptid.lwp ();
-  if (tid < 0)
-    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
+  lwp = ptid.lwp ();
+  if (lwp < 0)
+    buf += xsnprintf (buf, endbuf - buf, "-%lx", -lwp);
   else
-    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
+    buf += xsnprintf (buf, endbuf - buf, "%lx", lwp);
 
   return buf;
 }
@@ -3492,24 +3493,38 @@ read_ptid (const char *buf, const char **obuf)
 {
   const char *p = buf;
   const char *pp;
-  ULONGEST pid = 0, tid = 0;
+  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, &pid);
-      if (*pp != '.')
-	error (_("invalid remote ptid: %s"), p);
+      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);
 
-      p = pp;
-      pp = unpack_varlen_hex (p + 1, &tid);
       if (obuf)
 	*obuf = pp;
-      return ptid_t (pid, tid);
+
+      return ptid_t (pid, lwp);
     }
 
-  /* No multi-process.  Just a tid.  */
-  pp = unpack_varlen_hex (p, &tid);
+  /* 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)
@@ -3519,6 +3534,10 @@ read_ptid (const char *buf, const char **obuf)
       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
@@ -3531,7 +3550,8 @@ read_ptid (const char *buf, const char **obuf)
 
   if (obuf)
     *obuf = pp;
-  return ptid_t (pid, tid);
+
+  return ptid_t (pid, lwp);
 }
 
 static int
diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index f5d70c546d3..fe2b6acb4e9 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -519,7 +519,8 @@ char *
 write_ptid (char *buf, ptid_t ptid)
 {
   client_state &cs = get_client_state ();
-  int pid, tid;
+  ptid_t::pid_type pid;
+  ptid_t::lwp_type lwp;
 
   if (cs.multi_process)
     {
@@ -529,11 +530,11 @@ write_ptid (char *buf, ptid_t ptid)
       else
 	buf += sprintf (buf, "p%x.", pid);
     }
-  tid = ptid.lwp ();
-  if (tid < 0)
-    buf += sprintf (buf, "-%x", -tid);
+  lwp = ptid.lwp ();
+  if (lwp < 0)
+    buf += sprintf (buf, "-%lx", -lwp);
   else
-    buf += sprintf (buf, "%x", tid);
+    buf += sprintf (buf, "%lx", lwp);
 
   return buf;
 }
@@ -564,45 +565,59 @@ 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')
     {
-      ULONGEST pid;
-
       /* Multi-process ptid.  */
-      pp = unpack_varlen_hex (p + 1, &pid);
-      if (*pp != '.')
-	error ("invalid remote ptid: %s\n", p);
+      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);
 
-      ULONGEST tid = hex_or_minus_one (p, &pp);
+      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, tid);
+      return ptid_t (pid, lwp);
     }
 
-  /* No multi-process.  Just a tid.  */
-  ULONGEST tid = hex_or_minus_one (p, &pp);
+  /* No multi-process.  Just a thread id.  */
+  hex = hex_or_minus_one (p, &pp);
 
   /* Handle special thread ids.  */
-  if (tid == (ULONGEST) -1)
+  if (hex == (ULONGEST) -1)
     return minus_one_ptid;
 
-  if (tid == 0)
+  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.  */
-  int pid = get_first_process ()->pid;
+  pid = get_first_process ()->pid;
 
   if (obuf)
     *obuf = pp;
 
-  return ptid_t (pid, tid);
+  return ptid_t (pid, lwp);
 }
 
 /* Write COUNT bytes in BUF to the client.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928