Re: [PATCH] Warn users about mismatched PID namespaces

[Daniel Colascione <dancol@dancol.org>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Second version follows.

On 11/07/2014 12:49 PM, Pedro Alves wrote:
> On 10/30/2014 03:07 AM, Daniel Colascione wrote:
> 
>> diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
>> index 352fac1..4089417 100644
>> - --- a/gdb/linux-thread-db.c
>> +++ b/gdb/linux-thread-db.c
>> @@ -1223,6 +1223,25 @@ thread_db_new_objfile (struct objfile *objfile)
>>  static void
>>  thread_db_inferior_created (struct target_ops *target, int from_tty)
>>  {
> 
> This hook is called even if the current target isn't a native process.
> E.g., you may have loaded a core, in which case looking at
> getpid or /proc doesn't make sense.  Or you may be debugging with
> "target sim", or a remote process with gdbserver [1], etc.
> 
> We need this same check that thread_db_load does:
> 
>   /* Don't attempt to use thread_db for remote targets.  */
>   if (!(target_can_run (&current_target) || core_bfd))
>     return 0;
> 
> [1] BTW, could I interest in giving gdbserver/thread-db.c the
>     same treatment?

Do warnings from gdbserver even get propagated to remotes? I don't
see any obvious equivalent to the on-attach hook there.

>> +  /* If the child is in a different PID namespace, its idea of its PID
>> +     will differ from our idea of its PID.  When we scan the child's
>> +     thread list, we'll mistakenly think it has no threads since the
>> +     thread PID fields won't match the PID we give to
>> +     libthread_db.  */
>> +  char *our_pid_ns = linux_proc_pid_get_ns (getpid (), "pid");
>> +  char *inferior_pid_ns = linux_proc_pid_get_ns (
>> +    ptid_get_pid (inferior_ptid), "pid");
>> +
>> +  if (our_pid_ns != NULL && inferior_pid_ns != NULL &&
> 
> Put '&&' at the beginning of the next line.

Done

> 
>> +      strcmp (our_pid_ns, inferior_pid_ns) != 0)
> 
> 
>> +    {
>> +      warning (_ ("Target and debugger are in different PID namespaces; "
>> +		  "thread lists and other data are likely unreliable"));
>> +    }
>> +
>> +  xfree (our_pid_ns);
>> +  xfree (inferior_pid_ns);
> 
> Please factor this new code to a function; Having it in a function
> makes it easier to move the caller around if necessary.

Done.

> 
>> +
>>    check_for_thread_db ();
>>  }
>>
>> diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
>> index 30797da..8efccba 100644
>> - --- a/gdb/nat/linux-procfs.c
>> +++ b/gdb/nat/linux-procfs.c
>> @@ -113,3 +113,22 @@ linux_proc_pid_is_zombie (pid_t pid)
>>  {
>>    return linux_proc_pid_has_state (pid, "Z (zombie)");
>>  }
>> +
>> +/* See linux-procfs.h declaration.  */
>> +
>> +char*
>> +linux_proc_pid_get_ns (pid_t pid, const char *ns)
>> +{
>> +  char buf[100];
>> +  char nsval[64];
>> +  int ret;
>> +  snprintf (buf, sizeof (buf), "/proc/%d/ns/%s", (int) pid, ns);
> 
> Use xsnprintf

Done.

>>
>> +/* Return an opaque string identifying PID's NS namespace or NULL if
>> + * the information is unavailable.  The returned string must be
>> + * released with xfree.  */
>> +
>> +extern char* linux_proc_pid_get_ns (pid_t pid, const char *ns);
> 
> Space between char and *.

What do you mean? Lots of other functions in GDB use the style I used in my patch.


diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 352fac1..bd3635e 100644
- --- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1217,12 +1217,41 @@ thread_db_new_objfile (struct objfile *objfile)
     check_for_thread_db ();
 }
 
+static void
+check_pid_namespace_match ()
+{
+  /* Check is only relevant for local targets targets.  */
+  if (target_can_run (&current_target))
+    {
+      /* If the child is in a different PID namespace, its idea of its
+	 PID will differ from our idea of its PID.  When we scan the
+	 child's thread list, we'll mistakenly think it has no threads
+	 since the thread PID fields won't match the PID we give to
+	 libthread_db.  */
+      char *our_pid_ns = linux_proc_pid_get_ns (getpid (), "pid");
+      char *inferior_pid_ns = linux_proc_pid_get_ns (
+	ptid_get_pid (inferior_ptid), "pid");
+
+      if (our_pid_ns != NULL && inferior_pid_ns != NULL
+	  && strcmp (our_pid_ns, inferior_pid_ns) != 0)
+	{
+	  warning (_ ("Target and debugger are in different PID "
+		      "namespaces; thread lists and other data are "
+		      "likely unreliable"));
+	}
+
+      xfree (our_pid_ns);
+      xfree (inferior_pid_ns);
+    }
+}
+
 /* This function is called via the inferior_created observer.
    This handles the case of debugging statically linked executables.  */
 
 static void
 thread_db_inferior_created (struct target_ops *target, int from_tty)
 {
+  check_pid_namespace_match ();
   check_for_thread_db ();
 }
 
diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
index 30797da..8efccba 100644
- --- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -113,3 +113,22 @@ linux_proc_pid_is_zombie (pid_t pid)
 {
   return linux_proc_pid_has_state (pid, "Z (zombie)");
 }
+
+/* See linux-procfs.h declaration.  */
+
+char*
+linux_proc_pid_get_ns (pid_t pid, const char *ns)
+{
+  char buf[100];
+  char nsval[64];
+  int ret;
+  snprintf (buf, sizeof (buf), "/proc/%d/ns/%s", (int) pid, ns);
+  ret = readlink (buf, nsval, sizeof (nsval));
+  if (0 < ret && ret < sizeof (nsval))
+    {
+      nsval[ret] = '\0';
+      return xstrdup (nsval);
+    }
+
+  return NULL;
+}
diff --git a/gdb/nat/linux-procfs.h b/gdb/nat/linux-procfs.h
index d13fff7..e4f301f 100644
- --- a/gdb/nat/linux-procfs.h
+++ b/gdb/nat/linux-procfs.h
@@ -40,4 +40,10 @@ extern int linux_proc_pid_is_stopped (pid_t pid);
 
 extern int linux_proc_pid_is_zombie (pid_t pid);
 
+/* Return an opaque string identifying PID's NS namespace or NULL if
+ * the information is unavailable.  The returned string must be
+ * released with xfree.  */
+
+extern char* linux_proc_pid_get_ns (pid_t pid, const char *ns);
+
 #endif /* COMMON_LINUX_PROCFS_H */

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCAAGBQJUX9s8AAoJEN4WImmbpWBlApwP/2KmKdnBIh55ZDUggMOL63Ui
uFDcuccoQUH+WiLgVw36SJFLFOlDl4wab3ipg37eUiChR/iBXnAuBqHRIVTO5VfF
AkjXRAJw5obNcW0b62aD645gjQd4hOsvaYRDcuV6W+vbn1FIPxRxNGAHwtZQ7PNE
06vJu//OWbjQZVogGgVojDK7j1oWEm66nR2Se5UPOk+ddxqoFDtSCNAiutnhlstt
w4+Dh3NCDSJUnoL3qPQf8ex8qymojvwshTvJLOMVX7XzXnuGNXZg/8Au0SHAnTeI
CEwdrgw+SG3vXdp9BnzPn2EYzIiyu6lAfHp4HoRojmmMXhtk2ES+qg/W/Ty6Os6Q
wkPVa5W5TOygh6Ux213LyjJlnYl3fOFa3Hmg20Yu3AwmvIy4UDsgQjBqUouDhv2n
2FZPXZ+4XiRuK4zuPrXgfdP9ywYpshY/Yxaj4a+blHOMkViu3G8sWxKLDa90juAl
xjiYuevQHmTe6EOomuIfC/gzKszQhz9SnCpycgCtB9UCruHrgoFnHJYQwi760ynf
t7anmvUV7jWIUrH+2OQBEa+kEl3uM9fkQJa6x71bMfH6Ob8MYq7ciJv78uM38nfo
3KyGSsb4RR6aiLyLsBJIesmLfYKxMGm/uGd4IJJrvbf3mCH2YoNLPZNPCSKrUXfv
tD41GDUaXGSEYKBHYkSg
=bck+
-----END PGP SIGNATURE-----