[PATCH] Fix two bugs in gdbserver thread name handling

[PATCH] Fix two bugs in gdbserver thread name handling

[Tom Tromey]

Simon pointed out that my earlier patch to gdbserver's thread name
code:

    commit 07b3255c3bae7126a0d679f957788560351eb236
    Author: Tom Tromey <tom@tromey.com>
    Date:   Thu Jul 13 17:28:48 2023 -0600

	Filter invalid encodings from Linux thread names

... introduced a regression.  Looking at it, I found another bug as
well.  This patch fixes both of them.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31153
---
 gdbserver/linux-low.cc | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 4aa011c14ec..8cbc7833e53 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -7013,11 +7013,13 @@ current_lwp_ptid (void)
 }
 
 /* A helper function that copies NAME to DEST, replacing non-printable
-   characters with '?'.  Returns DEST as a convenience.  */
+   characters with '?'.  Returns the original DEST as a
+   convenience.  */
 
 static const char *
 replace_non_ascii (char *dest, const char *name)
 {
+  const char *result = dest;
   while (*name != '\0')
     {
       if (!ISPRINT (*name))
@@ -7026,7 +7028,8 @@ replace_non_ascii (char *dest, const char *name)
 	*dest++ = *name;
       ++name;
     }
-  return dest;
+  *dest = '\0';
+  return result;
 }
 
 const char *
@@ -7064,8 +7067,8 @@ linux_process_target::thread_name (ptid_t thread)
       else if ((errno == EILSEQ || errno == EINVAL)
 	       && outbuf < &dest[sizeof (dest) - 2])
 	*outbuf++ = '?';
-      *outbuf = '\0';
     }
+  *outbuf = '\0';
 
   iconv_close (handle);
   return *dest == '\0' ? nullptr : dest;
-- 
2.43.0

Re: [PATCH] Fix two bugs in gdbserver thread name handling

[Simon Marchi]

On 12/15/23 09:59, Tom Tromey wrote:
> Simon pointed out that my earlier patch to gdbserver's thread name
> code:
> 
>     commit 07b3255c3bae7126a0d679f957788560351eb236
>     Author: Tom Tromey <tom@tromey.com>
>     Date:   Thu Jul 13 17:28:48 2023 -0600
> 
> 	Filter invalid encodings from Linux thread names
> 
> ... introduced a regression.  Looking at it, I found another bug as
> well.  This patch fixes both of them.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31153

My intuition is that the changes make sense, but it would be nice if you
could describe the bugs in the commit message, to make sure we're on the
same page.

Simon

Re: [PATCH] Fix two bugs in gdbserver thread name handling

[Tom Tromey]

Simon> My intuition is that the changes make sense, but it would be nice if you
Simon> could describe the bugs in the commit message, to make sure we're on the
Simon> same page.

How's this?

Tom

commit 1d583fc640a2717b4e9d37d44602811ea201546f
Author: Tom Tromey <tromey@adacore.com>
Date:   Fri Dec 15 07:56:45 2023 -0700

    Fix two bugs in gdbserver thread name handling
    
    Simon pointed out that my earlier patch to gdbserver's thread name
    code:
    
        commit 07b3255c3bae7126a0d679f957788560351eb236
        Author: Tom Tromey <tom@tromey.com>
        Date:   Thu Jul 13 17:28:48 2023 -0600
    
            Filter invalid encodings from Linux thread names
    
    ... introduced a regression.  This bug was that the iconv output was
    not \0-terminated.
    
    Looking at it, I found another bug as well -- replace_non_ascii would
    not \0-terminate, and also would return the wrong pointer
    
    This patch fixes both of them.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31153

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 4aa011c14ec..8cbc7833e53 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -7013,11 +7013,13 @@ current_lwp_ptid (void)
 }
 
 /* A helper function that copies NAME to DEST, replacing non-printable
-   characters with '?'.  Returns DEST as a convenience.  */
+   characters with '?'.  Returns the original DEST as a
+   convenience.  */
 
 static const char *
 replace_non_ascii (char *dest, const char *name)
 {
+  const char *result = dest;
   while (*name != '\0')
     {
       if (!ISPRINT (*name))
@@ -7026,7 +7028,8 @@ replace_non_ascii (char *dest, const char *name)
 	*dest++ = *name;
       ++name;
     }
-  return dest;
+  *dest = '\0';
+  return result;
 }
 
 const char *
@@ -7064,8 +7067,8 @@ linux_process_target::thread_name (ptid_t thread)
       else if ((errno == EILSEQ || errno == EINVAL)
 	       && outbuf < &dest[sizeof (dest) - 2])
 	*outbuf++ = '?';
-      *outbuf = '\0';
     }
+  *outbuf = '\0';
 
   iconv_close (handle);
   return *dest == '\0' ? nullptr : dest;

Re: [PATCH] Fix two bugs in gdbserver thread name handling

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Simon> My intuition is that the changes make sense, but it would be nice if you
Simon> could describe the bugs in the commit message, to make sure we're on the
Simon> same page.

Tom> How's this?
[...]
Tom>     Fix two bugs in gdbserver thread name handling
    
I'm checking in the updated version of this patch.

Tom