diff mbox series

Fix two bugs in gdbserver thread name handling

Message ID 20231215145928.3396394-1-tromey@adacore.com
State New
Headers
Series Fix two bugs in gdbserver thread name handling |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Tom Tromey Dec. 15, 2023, 2:59 p.m. UTC 
  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(-)

Comments

Simon Marchi Dec. 15, 2023, 3:53 p.m. UTC | #1 
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
Tom Tromey Dec. 15, 2023, 4:24 p.m. UTC | #2 
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;
Tom Tromey Jan. 9, 2024, 2:14 p.m. UTC | #3 
>>>>> "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
diff mbox series

Patch

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;