Re: [PATCH] Warn users about mismatched PID namespaces

Daniel Colascione
2014-11-09T21:23:22Z

-----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-----