[PATCH 0/2][PR GDB/32956] gdb: fix GDB failing to find build-id debug files in linux mount namespaces

[PATCH 0/2][PR GDB/32956] gdb: fix GDB failing to find build-id debug files in linux mount namespaces

[Fabian Kilger]

GDB can no longer find build-id debug files in docker containers.
The cause is the new algorithm to look for a build-id-based debug file
(introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba).
It makes use of fileio_stat. As fileio_stat is not supported by
gdb/nat/linux-namespace.c, all stat calls would be performed on the
host and not inside the mount namespace. Furthermore, gdb/build-id.c
calls target_fileio_stat with inferior none, thus stat will always be
performed on the host mount namespace instead of the target's mount
namespace.

The patch is split into two parts: First, add support for fileio_stat
querying the linux mount namespace and second, adjust gdb/build-id.c
to call target_fileio_stat with current_inferior () instead of nullptr.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32956


Fabian Kilger (2):
  gdb: implement linux namespace support for fileio_stat
  gdb: query inferior's filesystem for build-id debug files

 gdb/build-id.c             |  6 ++--
 gdb/linux-nat.c            | 14 ++++++++
 gdb/linux-nat.h            |  3 ++
 gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
 gdb/nat/linux-namespaces.h |  6 ++++
 5 files changed, 98 insertions(+), 2 deletions(-)

-- 
2.49.0

[PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Fabian Kilger]

The new algorithm to look for a build-id-based debug file
(introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
makes use of fileio_stat. As fileio_stat was not supported by
linux-namespace.c, all stat calls would be performed on the host
and not inside the namespace

---
 gdb/linux-nat.c            | 14 ++++++++
 gdb/linux-nat.h            |  3 ++
 gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
 gdb/nat/linux-namespaces.h |  6 ++++
 4 files changed, 94 insertions(+)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 3f252370c7b..478a7977c4d 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
   return fd;
 }
 
+/* Implementation of to_fileio_stat.  */
+int
+linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
+			       struct stat *sb, fileio_error *target_errno)
+{
+  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
+			    filename, sb);
+
+  if (r == -1)
+    *target_errno = host_to_fileio_error (errno);
+
+  return r;
+}
+
 /* Implementation of to_fileio_readlink.  */
 
 std::optional<std::string>
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index b630b858e34..42d1ec142b3 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
 		     const char *filename,
 		     fileio_error *target_errno) override;
 
+  int fileio_stat (struct inferior *inf, const char *filename,
+		   struct stat *sb, fileio_error *target_errno) override;
+
   int fileio_unlink (struct inferior *inf,
 		     const char *filename,
 		     fileio_error *target_errno) override;
diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
index 19a05eec905..aa74e9df950 100644
--- a/gdb/nat/linux-namespaces.c
+++ b/gdb/nat/linux-namespaces.c
@@ -233,6 +233,12 @@ enum mnsh_msg_type
        MNSH_RET_INT.  */
     MNSH_REQ_SETNS,
 
+    /* A request that the helper call stat.  The single
+       argument (the filename) should be passed in BUF, and
+       should include a terminating NUL character.  The helper
+       should respond with a MNSH_RET_INTSTR.  */
+    MNSH_REQ_STAT,
+
     /* A request that the helper call open.  Arguments should
        be passed in BUF, INT1 and INT2.  The filename (in BUF)
        should include a terminating NUL character.  The helper
@@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
       debug_printf ("ERROR");
       break;
 
+    case MNSH_REQ_STAT:
+      debug_printf ("STAT");
+      break;
+
     case MNSH_REQ_SETNS:
       debug_printf ("SETNS");
       break;
@@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
   return mnsh_return_int (sock, result, errno);
 }
 
+
+/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
+
+static ssize_t
+mnsh_handle_stat(int sock, const char *filename)
+{
+  struct stat sb;
+  int stat_ok = stat(filename, &sb);
+
+  return mnsh_return_intstr(sock, stat_ok, &sb,
+			    stat_ok == -1 ? 0 : sizeof (sb),
+			    errno);
+}
+
 /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
 
 static ssize_t
@@ -574,6 +598,11 @@ mnsh_main (int sock)
 		response = mnsh_handle_setns (sock, fd, int1);
 	      break;
 
+	    case MNSH_REQ_STAT:
+	      if (size > 0 && buf[size - 1] == '\0')
+		response = mnsh_handle_stat(sock, buf);
+	      break;
+
 	    case MNSH_REQ_OPEN:
 	      if (size > 0 && buf[size - 1] == '\0')
 		response = mnsh_handle_open (sock, buf, int1, int2);
@@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
   mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
 		     filename, strlen (filename) + 1)
 
+#define mnsh_send_stat(helper, filename) \
+  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
+		     filename, strlen (filename) + 1)
+
 #define mnsh_send_unlink(helper, filename) \
   mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
 		     filename, strlen (filename) + 1)
@@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
   return MNSH_FS_HELPER;
 }
 
+
+/* See nat/linux-namespaces.h.  */
+int
+linux_mntns_stat (pid_t pid, const char *filename,
+		  struct stat *sb)
+{
+  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
+  struct linux_mnsh *helper;
+  int stat_ok, error;
+  ssize_t size;
+
+  if (access == MNSH_FS_ERROR)
+    return -1;
+
+  if (access == MNSH_FS_DIRECT)
+    return stat(filename, sb);
+
+  gdb_assert (access == MNSH_FS_HELPER);
+
+  helper = linux_mntns_get_helper ();
+
+  size = mnsh_send_stat (helper, filename);
+  if (size < 0)
+    return -1;
+
+  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
+
+  if (size < 0)
+    {
+      stat_ok = -1;
+      errno = error;
+    }
+  else
+    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
+
+  return stat_ok;
+}
+
 /* See nat/linux-namespaces.h.  */
 
 int
diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
index 4327292950b..825cb27eb2d 100644
--- a/gdb/nat/linux-namespaces.h
+++ b/gdb/nat/linux-namespaces.h
@@ -58,6 +58,12 @@ enum linux_ns_type
 
 extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
 
+/* Like stat(2), but in the mount namespace of process
+   PID.  */
+
+extern int linux_mntns_stat (pid_t pid, const char *filename,
+			     struct stat *sb);
+
 /* Like gdb_open_cloexec, but in the mount namespace of process
    PID.  */
 
-- 
2.49.0

[PATCH 2/2][PR GDB/32956] gdb: query inferior's filesystem for build-id debug files

[Fabian Kilger]

This fixes a bug related to build-id files with linux namespaces.
Specifically, we expect the debug files to be present inside the container,
thus the container filesystem should be queried if the program is running
inside one.

---
 gdb/build-id.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/build-id.c b/gdb/build-id.c
index 43a80dd3978..abbd65a7c12 100644
--- a/gdb/build-id.c
+++ b/gdb/build-id.c
@@ -29,6 +29,7 @@
 #include "gdbsupport/scoped_fd.h"
 #include "debuginfod-support.h"
 #include "extension.h"
+#include "inferior.h"
 
 /* See build-id.h.  */
 
@@ -128,7 +129,8 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
 	  if (supports_target_stat != TRIBOOL_FALSE)
 	    {
 	      struct stat sb;
-	      int res = target_fileio_stat (nullptr, link_on_target, &sb,
+	      int res = target_fileio_stat (current_inferior (),
+					    link_on_target, &sb,
 					    &target_errno);
 
 	      if (res != 0 && target_errno != FILEIO_ENOSYS)
@@ -157,7 +159,7 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
 		 the path doesn't exist, but we just assume that anything
 		 other than EINVAL indicates the path doesn't exist.  */
 	      std::optional<std::string> link_target
-		= target_fileio_readlink (nullptr, link_on_target,
+		= target_fileio_readlink (current_inferior (), link_on_target,
 					  &target_errno);
 	      if (link_target.has_value ()
 		  || target_errno == FILEIO_EINVAL)
-- 
2.49.0

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Andrew Burgess]

Fabian Kilger <kilger@sec.in.tum.de> writes:

Thanks for working on this.  This mostly looks good, I have just a few
style issues that I think should be fixed, but it's all pretty minor.


> The new algorithm to look for a build-id-based debug file
> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
> makes use of fileio_stat. As fileio_stat was not supported by
> linux-namespace.c, all stat calls would be performed on the host
> and not inside the namespace
>
> ---
>  gdb/linux-nat.c            | 14 ++++++++
>  gdb/linux-nat.h            |  3 ++
>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>  gdb/nat/linux-namespaces.h |  6 ++++
>  4 files changed, 94 insertions(+)
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 3f252370c7b..478a7977c4d 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>    return fd;
>  }
>  
> +/* Implementation of to_fileio_stat.  */
> +int

Add an empty line after the comment please to match the other functions
in this file.

> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
> +			       struct stat *sb, fileio_error *target_errno)
> +{
> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
> +			    filename, sb);

There's no need to wrap this argument list.

> +
> +  if (r == -1)
> +    *target_errno = host_to_fileio_error (errno);
> +
> +  return r;
> +}
> +
>  /* Implementation of to_fileio_readlink.  */
>  
>  std::optional<std::string>
> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
> index b630b858e34..42d1ec142b3 100644
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>  		     const char *filename,
>  		     fileio_error *target_errno) override;
>  
> +  int fileio_stat (struct inferior *inf, const char *filename,
> +		   struct stat *sb, fileio_error *target_errno) override;
> +
>    int fileio_unlink (struct inferior *inf,
>  		     const char *filename,
>  		     fileio_error *target_errno) override;
> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
> index 19a05eec905..aa74e9df950 100644
> --- a/gdb/nat/linux-namespaces.c
> +++ b/gdb/nat/linux-namespaces.c
> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>         MNSH_RET_INT.  */
>      MNSH_REQ_SETNS,
>  
> +    /* A request that the helper call stat.  The single
> +       argument (the filename) should be passed in BUF, and
> +       should include a terminating NUL character.  The helper
> +       should respond with a MNSH_RET_INTSTR.  */
> +    MNSH_REQ_STAT,
> +
>      /* A request that the helper call open.  Arguments should
>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>         should include a terminating NUL character.  The helper
> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>        debug_printf ("ERROR");
>        break;
>  
> +    case MNSH_REQ_STAT:
> +      debug_printf ("STAT");
> +      break;
> +
>      case MNSH_REQ_SETNS:
>        debug_printf ("SETNS");
>        break;
> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>    return mnsh_return_int (sock, result, errno);
>  }
>  
> +
> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
> +
> +static ssize_t
> +mnsh_handle_stat(int sock, const char *filename)
> +{
> +  struct stat sb;
> +  int stat_ok = stat(filename, &sb);

Space needed after 'stat'.

> +
> +  return mnsh_return_intstr(sock, stat_ok, &sb,
> +			    stat_ok == -1 ? 0 : sizeof (sb),
> +			    errno);

Space needed after 'mnsh_return_intstr'.

> +}
> +
>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>  
>  static ssize_t
> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>  		response = mnsh_handle_setns (sock, fd, int1);
>  	      break;
>  
> +	    case MNSH_REQ_STAT:
> +	      if (size > 0 && buf[size - 1] == '\0')
> +		response = mnsh_handle_stat(sock, buf);

Space needed after 'mnsh_handle_stat'

> +	      break;
> +
>  	    case MNSH_REQ_OPEN:
>  	      if (size > 0 && buf[size - 1] == '\0')
>  		response = mnsh_handle_open (sock, buf, int1, int2);
> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>  		     filename, strlen (filename) + 1)
>  
> +#define mnsh_send_stat(helper, filename) \
> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
> +		     filename, strlen (filename) + 1)
> +
>  #define mnsh_send_unlink(helper, filename) \
>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>  		     filename, strlen (filename) + 1)
> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>    return MNSH_FS_HELPER;
>  }
>  
> +
> +/* See nat/linux-namespaces.h.  */
> +int

Move the empty line from before the comment to after the comment please.

> +linux_mntns_stat (pid_t pid, const char *filename,
> +		  struct stat *sb)
> +{
> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
> +  struct linux_mnsh *helper;
> +  int stat_ok, error;
> +  ssize_t size;

The declarations should be moved inline below.

> +
> +  if (access == MNSH_FS_ERROR)
> +    return -1;
> +
> +  if (access == MNSH_FS_DIRECT)
> +    return stat(filename, sb);

Space needed after 'stat'.

> +
> +  gdb_assert (access == MNSH_FS_HELPER);
> +
> +  helper = linux_mntns_get_helper ();
> +
> +  size = mnsh_send_stat (helper, filename);
> +  if (size < 0)
> +    return -1;
> +
> +  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
> +
> +  if (size < 0)
> +    {
> +      stat_ok = -1;
> +      errno = error;
> +    }
> +  else
> +    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
> +
> +  return stat_ok;
> +}
> +
>  /* See nat/linux-namespaces.h.  */
>  
>  int
> diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
> index 4327292950b..825cb27eb2d 100644
> --- a/gdb/nat/linux-namespaces.h
> +++ b/gdb/nat/linux-namespaces.h
> @@ -58,6 +58,12 @@ enum linux_ns_type
>  
>  extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
>  
> +/* Like stat(2), but in the mount namespace of process
> +   PID.  */

This comment doesn't need to wrap.

Thanks,
Andrew



> +
> +extern int linux_mntns_stat (pid_t pid, const char *filename,
> +			     struct stat *sb);
> +
>  /* Like gdb_open_cloexec, but in the mount namespace of process
>     PID.  */
>  
> -- 
> 2.49.0

Re: [PATCH 2/2][PR GDB/32956] gdb: query inferior's filesystem for build-id debug files

[Andrew Burgess]

Fabian Kilger <kilger@sec.in.tum.de> writes:

> This fixes a bug related to build-id files with linux namespaces.
> Specifically, we expect the debug files to be present inside the container,
> thus the container filesystem should be queried if the program is running
> inside one.

You should add a bug link to the end of this commit message, like:

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32956

With that added, for this patch:

Approved-By: Andrew Burgess <aburgess@redhat.com>

You are welcome to merge this ahead of patch #1 if you want, as they are
related, but don't depend on each other.

Thanks,
Andrew



>
> ---
>  gdb/build-id.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/build-id.c b/gdb/build-id.c
> index 43a80dd3978..abbd65a7c12 100644
> --- a/gdb/build-id.c
> +++ b/gdb/build-id.c
> @@ -29,6 +29,7 @@
>  #include "gdbsupport/scoped_fd.h"
>  #include "debuginfod-support.h"
>  #include "extension.h"
> +#include "inferior.h"
>  
>  /* See build-id.h.  */
>  
> @@ -128,7 +129,8 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
>  	  if (supports_target_stat != TRIBOOL_FALSE)
>  	    {
>  	      struct stat sb;
> -	      int res = target_fileio_stat (nullptr, link_on_target, &sb,
> +	      int res = target_fileio_stat (current_inferior (),
> +					    link_on_target, &sb,
>  					    &target_errno);
>  
>  	      if (res != 0 && target_errno != FILEIO_ENOSYS)
> @@ -157,7 +159,7 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
>  		 the path doesn't exist, but we just assume that anything
>  		 other than EINVAL indicates the path doesn't exist.  */
>  	      std::optional<std::string> link_target
> -		= target_fileio_readlink (nullptr, link_on_target,
> +		= target_fileio_readlink (current_inferior (), link_on_target,
>  					  &target_errno);
>  	      if (link_target.has_value ()
>  		  || target_errno == FILEIO_EINVAL)
> -- 
> 2.49.0

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Andrew Burgess]

Fabian Kilger <kilger@sec.in.tum.de> writes:

> The new algorithm to look for a build-id-based debug file
> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
> makes use of fileio_stat. As fileio_stat was not supported by
> linux-namespace.c, all stat calls would be performed on the host
> and not inside the namespace
>
> ---
>  gdb/linux-nat.c            | 14 ++++++++
>  gdb/linux-nat.h            |  3 ++
>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>  gdb/nat/linux-namespaces.h |  6 ++++

Thinking about this some more, I realised that this is missing the
gdbserver related changes.

If you search in the gdbserver/ directory for how multifs_readlink is
used then this will give a pretty good idea for how a new multifs_stat
should be added.

For manual testing things will be pretty similar to testing GDB.  On the
same host as your container, but outside of the container, start
gdbserver:

  $ gdbserver --multi --once :54321

Then on the same host, start GDB, and within GDB:

  (gdb) target extended-remote :54321
  (gdb) attach PID

I'm assuming that currently you are just attaching directly from GDB to
a process within the container?

Thanks,
Andrew

>  4 files changed, 94 insertions(+)
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 3f252370c7b..478a7977c4d 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>    return fd;
>  }
>  
> +/* Implementation of to_fileio_stat.  */
> +int
> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
> +			       struct stat *sb, fileio_error *target_errno)
> +{
> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
> +			    filename, sb);
> +
> +  if (r == -1)
> +    *target_errno = host_to_fileio_error (errno);
> +
> +  return r;
> +}
> +
>  /* Implementation of to_fileio_readlink.  */
>  
>  std::optional<std::string>
> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
> index b630b858e34..42d1ec142b3 100644
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>  		     const char *filename,
>  		     fileio_error *target_errno) override;
>  
> +  int fileio_stat (struct inferior *inf, const char *filename,
> +		   struct stat *sb, fileio_error *target_errno) override;
> +
>    int fileio_unlink (struct inferior *inf,
>  		     const char *filename,
>  		     fileio_error *target_errno) override;
> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
> index 19a05eec905..aa74e9df950 100644
> --- a/gdb/nat/linux-namespaces.c
> +++ b/gdb/nat/linux-namespaces.c
> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>         MNSH_RET_INT.  */
>      MNSH_REQ_SETNS,
>  
> +    /* A request that the helper call stat.  The single
> +       argument (the filename) should be passed in BUF, and
> +       should include a terminating NUL character.  The helper
> +       should respond with a MNSH_RET_INTSTR.  */
> +    MNSH_REQ_STAT,
> +
>      /* A request that the helper call open.  Arguments should
>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>         should include a terminating NUL character.  The helper
> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>        debug_printf ("ERROR");
>        break;
>  
> +    case MNSH_REQ_STAT:
> +      debug_printf ("STAT");
> +      break;
> +
>      case MNSH_REQ_SETNS:
>        debug_printf ("SETNS");
>        break;
> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>    return mnsh_return_int (sock, result, errno);
>  }
>  
> +
> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
> +
> +static ssize_t
> +mnsh_handle_stat(int sock, const char *filename)
> +{
> +  struct stat sb;
> +  int stat_ok = stat(filename, &sb);
> +
> +  return mnsh_return_intstr(sock, stat_ok, &sb,
> +			    stat_ok == -1 ? 0 : sizeof (sb),
> +			    errno);
> +}
> +
>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>  
>  static ssize_t
> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>  		response = mnsh_handle_setns (sock, fd, int1);
>  	      break;
>  
> +	    case MNSH_REQ_STAT:
> +	      if (size > 0 && buf[size - 1] == '\0')
> +		response = mnsh_handle_stat(sock, buf);
> +	      break;
> +
>  	    case MNSH_REQ_OPEN:
>  	      if (size > 0 && buf[size - 1] == '\0')
>  		response = mnsh_handle_open (sock, buf, int1, int2);
> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>  		     filename, strlen (filename) + 1)
>  
> +#define mnsh_send_stat(helper, filename) \
> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
> +		     filename, strlen (filename) + 1)
> +
>  #define mnsh_send_unlink(helper, filename) \
>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>  		     filename, strlen (filename) + 1)
> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>    return MNSH_FS_HELPER;
>  }
>  
> +
> +/* See nat/linux-namespaces.h.  */
> +int
> +linux_mntns_stat (pid_t pid, const char *filename,
> +		  struct stat *sb)
> +{
> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
> +  struct linux_mnsh *helper;
> +  int stat_ok, error;
> +  ssize_t size;
> +
> +  if (access == MNSH_FS_ERROR)
> +    return -1;
> +
> +  if (access == MNSH_FS_DIRECT)
> +    return stat(filename, sb);
> +
> +  gdb_assert (access == MNSH_FS_HELPER);
> +
> +  helper = linux_mntns_get_helper ();
> +
> +  size = mnsh_send_stat (helper, filename);
> +  if (size < 0)
> +    return -1;
> +
> +  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
> +
> +  if (size < 0)
> +    {
> +      stat_ok = -1;
> +      errno = error;
> +    }
> +  else
> +    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
> +
> +  return stat_ok;
> +}
> +
>  /* See nat/linux-namespaces.h.  */
>  
>  int
> diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
> index 4327292950b..825cb27eb2d 100644
> --- a/gdb/nat/linux-namespaces.h
> +++ b/gdb/nat/linux-namespaces.h
> @@ -58,6 +58,12 @@ enum linux_ns_type
>  
>  extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
>  
> +/* Like stat(2), but in the mount namespace of process
> +   PID.  */
> +
> +extern int linux_mntns_stat (pid_t pid, const char *filename,
> +			     struct stat *sb);
> +
>  /* Like gdb_open_cloexec, but in the mount namespace of process
>     PID.  */
>  
> -- 
> 2.49.0

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Fabian Kilger]

[-- Attachment #1.1.1: Type: text/plain, Size: 7539 bytes --]

Hi Andrew,

you're right, I'm directly using GDB and wasn't aware of that gdbserver
module. I'll be adding it to the patch for v2.

Best,
Fabian

On 5/24/25 12:46, Andrew Burgess wrote:
> Fabian Kilger <kilger@sec.in.tum.de> writes:
> 
>> The new algorithm to look for a build-id-based debug file
>> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
>> makes use of fileio_stat. As fileio_stat was not supported by
>> linux-namespace.c, all stat calls would be performed on the host
>> and not inside the namespace
>>
>> ---
>>  gdb/linux-nat.c            | 14 ++++++++
>>  gdb/linux-nat.h            |  3 ++
>>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>>  gdb/nat/linux-namespaces.h |  6 ++++
> 
> Thinking about this some more, I realised that this is missing the
> gdbserver related changes.
> 
> If you search in the gdbserver/ directory for how multifs_readlink is
> used then this will give a pretty good idea for how a new multifs_stat
> should be added.
> 
> For manual testing things will be pretty similar to testing GDB.  On the
> same host as your container, but outside of the container, start
> gdbserver:
> 
>   $ gdbserver --multi --once :54321
> 
> Then on the same host, start GDB, and within GDB:
> 
>   (gdb) target extended-remote :54321
>   (gdb) attach PID
> 
> I'm assuming that currently you are just attaching directly from GDB to
> a process within the container?
> 
> Thanks,
> Andrew
> 
>>  4 files changed, 94 insertions(+)
>>
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index 3f252370c7b..478a7977c4d 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>>    return fd;
>>  }
>>  
>> +/* Implementation of to_fileio_stat.  */
>> +int
>> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
>> +			       struct stat *sb, fileio_error *target_errno)
>> +{
>> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
>> +			    filename, sb);
>> +
>> +  if (r == -1)
>> +    *target_errno = host_to_fileio_error (errno);
>> +
>> +  return r;
>> +}
>> +
>>  /* Implementation of to_fileio_readlink.  */
>>  
>>  std::optional<std::string>
>> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
>> index b630b858e34..42d1ec142b3 100644
>> --- a/gdb/linux-nat.h
>> +++ b/gdb/linux-nat.h
>> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>>  		     const char *filename,
>>  		     fileio_error *target_errno) override;
>>  
>> +  int fileio_stat (struct inferior *inf, const char *filename,
>> +		   struct stat *sb, fileio_error *target_errno) override;
>> +
>>    int fileio_unlink (struct inferior *inf,
>>  		     const char *filename,
>>  		     fileio_error *target_errno) override;
>> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
>> index 19a05eec905..aa74e9df950 100644
>> --- a/gdb/nat/linux-namespaces.c
>> +++ b/gdb/nat/linux-namespaces.c
>> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>>         MNSH_RET_INT.  */
>>      MNSH_REQ_SETNS,
>>  
>> +    /* A request that the helper call stat.  The single
>> +       argument (the filename) should be passed in BUF, and
>> +       should include a terminating NUL character.  The helper
>> +       should respond with a MNSH_RET_INTSTR.  */
>> +    MNSH_REQ_STAT,
>> +
>>      /* A request that the helper call open.  Arguments should
>>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>>         should include a terminating NUL character.  The helper
>> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>>        debug_printf ("ERROR");
>>        break;
>>  
>> +    case MNSH_REQ_STAT:
>> +      debug_printf ("STAT");
>> +      break;
>> +
>>      case MNSH_REQ_SETNS:
>>        debug_printf ("SETNS");
>>        break;
>> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>>    return mnsh_return_int (sock, result, errno);
>>  }
>>  
>> +
>> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
>> +
>> +static ssize_t
>> +mnsh_handle_stat(int sock, const char *filename)
>> +{
>> +  struct stat sb;
>> +  int stat_ok = stat(filename, &sb);
>> +
>> +  return mnsh_return_intstr(sock, stat_ok, &sb,
>> +			    stat_ok == -1 ? 0 : sizeof (sb),
>> +			    errno);
>> +}
>> +
>>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>>  
>>  static ssize_t
>> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>>  		response = mnsh_handle_setns (sock, fd, int1);
>>  	      break;
>>  
>> +	    case MNSH_REQ_STAT:
>> +	      if (size > 0 && buf[size - 1] == '\0')
>> +		response = mnsh_handle_stat(sock, buf);
>> +	      break;
>> +
>>  	    case MNSH_REQ_OPEN:
>>  	      if (size > 0 && buf[size - 1] == '\0')
>>  		response = mnsh_handle_open (sock, buf, int1, int2);
>> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>>  		     filename, strlen (filename) + 1)
>>  
>> +#define mnsh_send_stat(helper, filename) \
>> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
>> +		     filename, strlen (filename) + 1)
>> +
>>  #define mnsh_send_unlink(helper, filename) \
>>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>>  		     filename, strlen (filename) + 1)
>> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>>    return MNSH_FS_HELPER;
>>  }
>>  
>> +
>> +/* See nat/linux-namespaces.h.  */
>> +int
>> +linux_mntns_stat (pid_t pid, const char *filename,
>> +		  struct stat *sb)
>> +{
>> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
>> +  struct linux_mnsh *helper;
>> +  int stat_ok, error;
>> +  ssize_t size;
>> +
>> +  if (access == MNSH_FS_ERROR)
>> +    return -1;
>> +
>> +  if (access == MNSH_FS_DIRECT)
>> +    return stat(filename, sb);
>> +
>> +  gdb_assert (access == MNSH_FS_HELPER);
>> +
>> +  helper = linux_mntns_get_helper ();
>> +
>> +  size = mnsh_send_stat (helper, filename);
>> +  if (size < 0)
>> +    return -1;
>> +
>> +  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
>> +
>> +  if (size < 0)
>> +    {
>> +      stat_ok = -1;
>> +      errno = error;
>> +    }
>> +  else
>> +    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
>> +
>> +  return stat_ok;
>> +}
>> +
>>  /* See nat/linux-namespaces.h.  */
>>  
>>  int
>> diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
>> index 4327292950b..825cb27eb2d 100644
>> --- a/gdb/nat/linux-namespaces.h
>> +++ b/gdb/nat/linux-namespaces.h
>> @@ -58,6 +58,12 @@ enum linux_ns_type
>>  
>>  extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
>>  
>> +/* Like stat(2), but in the mount namespace of process
>> +   PID.  */
>> +
>> +extern int linux_mntns_stat (pid_t pid, const char *filename,
>> +			     struct stat *sb);
>> +
>>  /* Like gdb_open_cloexec, but in the mount namespace of process
>>     PID.  */
>>  
>> -- 
>> 2.49.0
> 

-- 
Fabian Kilger, M.Sc.
Wissenschaftlicher Mitarbeiter

Technische Universität München
TUM School of Computation, Information and Technology
Chair of IT Security

Boltzmannstraße 3
85748 Garching (bei München)

Tel. +49 (0)89 289-18587
Fax +49 (0)89 289-18579

kilger@sec.in.tum.de
www.sec.in.tum.de

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 653 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 2/2][PR GDB/32956] gdb: query inferior's filesystem for build-id debug files

[Fabian Kilger]

[-- Attachment #1.1.1: Type: text/plain, Size: 2697 bytes --]

Thanks for approving, I'll add the link to the commit message.

If I merge this ahead of #1, can I leave it out in the v2 patch
submission, or should it be included for completeness' sake - not sure
what is desired.

Best,
Fabian

On 5/23/25 20:20, Andrew Burgess wrote:
> Fabian Kilger <kilger@sec.in.tum.de> writes:
> 
>> This fixes a bug related to build-id files with linux namespaces.
>> Specifically, we expect the debug files to be present inside the container,
>> thus the container filesystem should be queried if the program is running
>> inside one.
> 
> You should add a bug link to the end of this commit message, like:
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32956
> 
> With that added, for this patch:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>
> 
> You are welcome to merge this ahead of patch #1 if you want, as they are
> related, but don't depend on each other.
> 
> Thanks,
> Andrew
> 
> 
> 
>>
>> ---
>>  gdb/build-id.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/build-id.c b/gdb/build-id.c
>> index 43a80dd3978..abbd65a7c12 100644
>> --- a/gdb/build-id.c
>> +++ b/gdb/build-id.c
>> @@ -29,6 +29,7 @@
>>  #include "gdbsupport/scoped_fd.h"
>>  #include "debuginfod-support.h"
>>  #include "extension.h"
>> +#include "inferior.h"
>>  
>>  /* See build-id.h.  */
>>  
>> @@ -128,7 +129,8 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
>>  	  if (supports_target_stat != TRIBOOL_FALSE)
>>  	    {
>>  	      struct stat sb;
>> -	      int res = target_fileio_stat (nullptr, link_on_target, &sb,
>> +	      int res = target_fileio_stat (current_inferior (),
>> +					    link_on_target, &sb,
>>  					    &target_errno);
>>  
>>  	      if (res != 0 && target_errno != FILEIO_ENOSYS)
>> @@ -157,7 +159,7 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
>>  		 the path doesn't exist, but we just assume that anything
>>  		 other than EINVAL indicates the path doesn't exist.  */
>>  	      std::optional<std::string> link_target
>> -		= target_fileio_readlink (nullptr, link_on_target,
>> +		= target_fileio_readlink (current_inferior (), link_on_target,
>>  					  &target_errno);
>>  	      if (link_target.has_value ()
>>  		  || target_errno == FILEIO_EINVAL)
>> -- 
>> 2.49.0
> 

-- 
Fabian Kilger, M.Sc.
Wissenschaftlicher Mitarbeiter

Technische Universität München
TUM School of Computation, Information and Technology
Chair of IT Security

Boltzmannstraße 3
85748 Garching (bei München)

Tel. +49 (0)89 289-18587
Fax +49 (0)89 289-18579

kilger@sec.in.tum.de
www.sec.in.tum.de

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 653 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Fabian Kilger]

[-- Attachment #1.1.1: Type: text/plain, Size: 8090 bytes --]

Thanks for reviewing, I'll be including these in v2. I have questions
regarding some style recommendations, where I feel like I'd be breaking
the style of the remaining file.

On 5/23/25 20:14, Andrew Burgess wrote:
> Fabian Kilger <kilger@sec.in.tum.de> writes:
> 
> Thanks for working on this.  This mostly looks good, I have just a few
> style issues that I think should be fixed, but it's all pretty minor.
> 
> 
>> The new algorithm to look for a build-id-based debug file
>> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
>> makes use of fileio_stat. As fileio_stat was not supported by
>> linux-namespace.c, all stat calls would be performed on the host
>> and not inside the namespace
>>
>> ---
>>  gdb/linux-nat.c            | 14 ++++++++
>>  gdb/linux-nat.h            |  3 ++
>>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>>  gdb/nat/linux-namespaces.h |  6 ++++
>>  4 files changed, 94 insertions(+)
>>
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index 3f252370c7b..478a7977c4d 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>>    return fd;
>>  }
>>  
>> +/* Implementation of to_fileio_stat.  */
>> +int
> 
> Add an empty line after the comment please to match the other functions
> in this file.
> 
>> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
>> +			       struct stat *sb, fileio_error *target_errno)
>> +{
>> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
>> +			    filename, sb);
> 
> There's no need to wrap this argument list.
> 

I've tried copying the style of the other functions in the file. All
fileio_X functions wrap the argument list after linux_nat_fileio_pid_of.
Specifically also fileio_unlink, which has even less arguments. As such,
I feel like not wrapping would break with the style of the other
functions. Should I still not wrap it?

>> +
>> +  if (r == -1)
>> +    *target_errno = host_to_fileio_error (errno);
>> +
>> +  return r;
>> +}
>> +
>>  /* Implementation of to_fileio_readlink.  */
>>  
>>  std::optional<std::string>
>> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
>> index b630b858e34..42d1ec142b3 100644
>> --- a/gdb/linux-nat.h
>> +++ b/gdb/linux-nat.h
>> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>>  		     const char *filename,
>>  		     fileio_error *target_errno) override;
>>  
>> +  int fileio_stat (struct inferior *inf, const char *filename,
>> +		   struct stat *sb, fileio_error *target_errno) override;
>> +
>>    int fileio_unlink (struct inferior *inf,
>>  		     const char *filename,
>>  		     fileio_error *target_errno) override;
>> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
>> index 19a05eec905..aa74e9df950 100644
>> --- a/gdb/nat/linux-namespaces.c
>> +++ b/gdb/nat/linux-namespaces.c
>> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>>         MNSH_RET_INT.  */
>>      MNSH_REQ_SETNS,
>>  
>> +    /* A request that the helper call stat.  The single
>> +       argument (the filename) should be passed in BUF, and
>> +       should include a terminating NUL character.  The helper
>> +       should respond with a MNSH_RET_INTSTR.  */
>> +    MNSH_REQ_STAT,
>> +
>>      /* A request that the helper call open.  Arguments should
>>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>>         should include a terminating NUL character.  The helper
>> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>>        debug_printf ("ERROR");
>>        break;
>>  
>> +    case MNSH_REQ_STAT:
>> +      debug_printf ("STAT");
>> +      break;
>> +
>>      case MNSH_REQ_SETNS:
>>        debug_printf ("SETNS");
>>        break;
>> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>>    return mnsh_return_int (sock, result, errno);
>>  }
>>  
>> +
>> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
>> +
>> +static ssize_t
>> +mnsh_handle_stat(int sock, const char *filename)
>> +{
>> +  struct stat sb;
>> +  int stat_ok = stat(filename, &sb);
> 
> Space needed after 'stat'.
> 
>> +
>> +  return mnsh_return_intstr(sock, stat_ok, &sb,
>> +			    stat_ok == -1 ? 0 : sizeof (sb),
>> +			    errno);
> 
> Space needed after 'mnsh_return_intstr'.
> 
>> +}
>> +
>>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>>  
>>  static ssize_t
>> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>>  		response = mnsh_handle_setns (sock, fd, int1);
>>  	      break;
>>  
>> +	    case MNSH_REQ_STAT:
>> +	      if (size > 0 && buf[size - 1] == '\0')
>> +		response = mnsh_handle_stat(sock, buf);
> 
> Space needed after 'mnsh_handle_stat'
> 
>> +	      break;
>> +
>>  	    case MNSH_REQ_OPEN:
>>  	      if (size > 0 && buf[size - 1] == '\0')
>>  		response = mnsh_handle_open (sock, buf, int1, int2);
>> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>>  		     filename, strlen (filename) + 1)
>>  
>> +#define mnsh_send_stat(helper, filename) \
>> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
>> +		     filename, strlen (filename) + 1)
>> +
>>  #define mnsh_send_unlink(helper, filename) \
>>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>>  		     filename, strlen (filename) + 1)
>> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>>    return MNSH_FS_HELPER;
>>  }
>>  
>> +
>> +/* See nat/linux-namespaces.h.  */
>> +int
> 
> Move the empty line from before the comment to after the comment please.
> 
>> +linux_mntns_stat (pid_t pid, const char *filename,
>> +		  struct stat *sb)
>> +{
>> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
>> +  struct linux_mnsh *helper;
>> +  int stat_ok, error;
>> +  ssize_t size;
> 
> The declarations should be moved inline below.

Here I've also copied the style of the remaining functions: All
functions in that file declare the variables at the beginning in the
exact same fashion. See linux_mntns_open_cloexec right below it, for
example.

Best,
Fabian

> 
>> +
>> +  if (access == MNSH_FS_ERROR)
>> +    return -1;
>> +
>> +  if (access == MNSH_FS_DIRECT)
>> +    return stat(filename, sb);
> 
> Space needed after 'stat'.
> 
>> +
>> +  gdb_assert (access == MNSH_FS_HELPER);
>> +
>> +  helper = linux_mntns_get_helper ();
>> +
>> +  size = mnsh_send_stat (helper, filename);
>> +  if (size < 0)
>> +    return -1;
>> +
>> +  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
>> +
>> +  if (size < 0)
>> +    {
>> +      stat_ok = -1;
>> +      errno = error;
>> +    }
>> +  else
>> +    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
>> +
>> +  return stat_ok;
>> +}
>> +
>>  /* See nat/linux-namespaces.h.  */
>>  
>>  int
>> diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
>> index 4327292950b..825cb27eb2d 100644
>> --- a/gdb/nat/linux-namespaces.h
>> +++ b/gdb/nat/linux-namespaces.h
>> @@ -58,6 +58,12 @@ enum linux_ns_type
>>  
>>  extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
>>  
>> +/* Like stat(2), but in the mount namespace of process
>> +   PID.  */
> 
> This comment doesn't need to wrap.
> 
> Thanks,
> Andrew
> 
> 
> 
>> +
>> +extern int linux_mntns_stat (pid_t pid, const char *filename,
>> +			     struct stat *sb);
>> +
>>  /* Like gdb_open_cloexec, but in the mount namespace of process
>>     PID.  */
>>  
>> -- 
>> 2.49.0
> 

-- 
Fabian Kilger, M.Sc.
Wissenschaftlicher Mitarbeiter

Technische Universität München
TUM School of Computation, Information and Technology
Chair of IT Security

Boltzmannstraße 3
85748 Garching (bei München)

Tel. +49 (0)89 289-18587
Fax +49 (0)89 289-18579

kilger@sec.in.tum.de
www.sec.in.tum.de

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 653 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Fabian Kilger]

[-- Attachment #1.1.1: Type: text/plain, Size: 8264 bytes --]

While looking at it, I've noticed all implementations of stat functions
actually use lstat and not stat. This maybe should be modified in the
namespace case as well and I'll be changing this for v2. However, I
could not directly find a rationale behind why *_stat functions call
lstat instead. Possibly, it might make sense renaming the
target_fileio_stat to target_fileio_lstat as well, though this would be
an independent change.

Best,
Fabian

On 5/24/25 21:43, Fabian Kilger wrote:
> Hi Andrew,
> 
> you're right, I'm directly using GDB and wasn't aware of that gdbserver
> module. I'll be adding it to the patch for v2.
> 
> Best,
> Fabian
> 
> On 5/24/25 12:46, Andrew Burgess wrote:
>> Fabian Kilger <kilger@sec.in.tum.de> writes:
>>
>>> The new algorithm to look for a build-id-based debug file
>>> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
>>> makes use of fileio_stat. As fileio_stat was not supported by
>>> linux-namespace.c, all stat calls would be performed on the host
>>> and not inside the namespace
>>>
>>> ---
>>>  gdb/linux-nat.c            | 14 ++++++++
>>>  gdb/linux-nat.h            |  3 ++
>>>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>>>  gdb/nat/linux-namespaces.h |  6 ++++
>>
>> Thinking about this some more, I realised that this is missing the
>> gdbserver related changes.
>>
>> If you search in the gdbserver/ directory for how multifs_readlink is
>> used then this will give a pretty good idea for how a new multifs_stat
>> should be added.
>>
>> For manual testing things will be pretty similar to testing GDB.  On the
>> same host as your container, but outside of the container, start
>> gdbserver:
>>
>>   $ gdbserver --multi --once :54321
>>
>> Then on the same host, start GDB, and within GDB:
>>
>>   (gdb) target extended-remote :54321
>>   (gdb) attach PID
>>
>> I'm assuming that currently you are just attaching directly from GDB to
>> a process within the container?
>>
>> Thanks,
>> Andrew
>>
>>>  4 files changed, 94 insertions(+)
>>>
>>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>>> index 3f252370c7b..478a7977c4d 100644
>>> --- a/gdb/linux-nat.c
>>> +++ b/gdb/linux-nat.c
>>> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>>>    return fd;
>>>  }
>>>  
>>> +/* Implementation of to_fileio_stat.  */
>>> +int
>>> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
>>> +			       struct stat *sb, fileio_error *target_errno)
>>> +{
>>> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
>>> +			    filename, sb);
>>> +
>>> +  if (r == -1)
>>> +    *target_errno = host_to_fileio_error (errno);
>>> +
>>> +  return r;
>>> +}
>>> +
>>>  /* Implementation of to_fileio_readlink.  */
>>>  
>>>  std::optional<std::string>
>>> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
>>> index b630b858e34..42d1ec142b3 100644
>>> --- a/gdb/linux-nat.h
>>> +++ b/gdb/linux-nat.h
>>> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>>>  		     const char *filename,
>>>  		     fileio_error *target_errno) override;
>>>  
>>> +  int fileio_stat (struct inferior *inf, const char *filename,
>>> +		   struct stat *sb, fileio_error *target_errno) override;
>>> +
>>>    int fileio_unlink (struct inferior *inf,
>>>  		     const char *filename,
>>>  		     fileio_error *target_errno) override;
>>> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
>>> index 19a05eec905..aa74e9df950 100644
>>> --- a/gdb/nat/linux-namespaces.c
>>> +++ b/gdb/nat/linux-namespaces.c
>>> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>>>         MNSH_RET_INT.  */
>>>      MNSH_REQ_SETNS,
>>>  
>>> +    /* A request that the helper call stat.  The single
>>> +       argument (the filename) should be passed in BUF, and
>>> +       should include a terminating NUL character.  The helper
>>> +       should respond with a MNSH_RET_INTSTR.  */
>>> +    MNSH_REQ_STAT,
>>> +
>>>      /* A request that the helper call open.  Arguments should
>>>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>>>         should include a terminating NUL character.  The helper
>>> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>>>        debug_printf ("ERROR");
>>>        break;
>>>  
>>> +    case MNSH_REQ_STAT:
>>> +      debug_printf ("STAT");
>>> +      break;
>>> +
>>>      case MNSH_REQ_SETNS:
>>>        debug_printf ("SETNS");
>>>        break;
>>> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>>>    return mnsh_return_int (sock, result, errno);
>>>  }
>>>  
>>> +
>>> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
>>> +
>>> +static ssize_t
>>> +mnsh_handle_stat(int sock, const char *filename)
>>> +{
>>> +  struct stat sb;
>>> +  int stat_ok = stat(filename, &sb);
>>> +
>>> +  return mnsh_return_intstr(sock, stat_ok, &sb,
>>> +			    stat_ok == -1 ? 0 : sizeof (sb),
>>> +			    errno);
>>> +}
>>> +
>>>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>>>  
>>>  static ssize_t
>>> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>>>  		response = mnsh_handle_setns (sock, fd, int1);
>>>  	      break;
>>>  
>>> +	    case MNSH_REQ_STAT:
>>> +	      if (size > 0 && buf[size - 1] == '\0')
>>> +		response = mnsh_handle_stat(sock, buf);
>>> +	      break;
>>> +
>>>  	    case MNSH_REQ_OPEN:
>>>  	      if (size > 0 && buf[size - 1] == '\0')
>>>  		response = mnsh_handle_open (sock, buf, int1, int2);
>>> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>>>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>>>  		     filename, strlen (filename) + 1)
>>>  
>>> +#define mnsh_send_stat(helper, filename) \
>>> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
>>> +		     filename, strlen (filename) + 1)
>>> +
>>>  #define mnsh_send_unlink(helper, filename) \
>>>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>>>  		     filename, strlen (filename) + 1)
>>> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>>>    return MNSH_FS_HELPER;
>>>  }
>>>  
>>> +
>>> +/* See nat/linux-namespaces.h.  */
>>> +int
>>> +linux_mntns_stat (pid_t pid, const char *filename,
>>> +		  struct stat *sb)
>>> +{
>>> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
>>> +  struct linux_mnsh *helper;
>>> +  int stat_ok, error;
>>> +  ssize_t size;
>>> +
>>> +  if (access == MNSH_FS_ERROR)
>>> +    return -1;
>>> +
>>> +  if (access == MNSH_FS_DIRECT)
>>> +    return stat(filename, sb);
>>> +
>>> +  gdb_assert (access == MNSH_FS_HELPER);
>>> +
>>> +  helper = linux_mntns_get_helper ();
>>> +
>>> +  size = mnsh_send_stat (helper, filename);
>>> +  if (size < 0)
>>> +    return -1;
>>> +
>>> +  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
>>> +
>>> +  if (size < 0)
>>> +    {
>>> +      stat_ok = -1;
>>> +      errno = error;
>>> +    }
>>> +  else
>>> +    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
>>> +
>>> +  return stat_ok;
>>> +}
>>> +
>>>  /* See nat/linux-namespaces.h.  */
>>>  
>>>  int
>>> diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
>>> index 4327292950b..825cb27eb2d 100644
>>> --- a/gdb/nat/linux-namespaces.h
>>> +++ b/gdb/nat/linux-namespaces.h
>>> @@ -58,6 +58,12 @@ enum linux_ns_type
>>>  
>>>  extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
>>>  
>>> +/* Like stat(2), but in the mount namespace of process
>>> +   PID.  */
>>> +
>>> +extern int linux_mntns_stat (pid_t pid, const char *filename,
>>> +			     struct stat *sb);
>>> +
>>>  /* Like gdb_open_cloexec, but in the mount namespace of process
>>>     PID.  */
>>>  
>>> -- 
>>> 2.49.0
>>
> 

-- 
Fabian Kilger, M.Sc.
Wissenschaftlicher Mitarbeiter

Technische Universität München
TUM School of Computation, Information and Technology
Chair of IT Security

Boltzmannstraße 3
85748 Garching (bei München)

Tel. +49 (0)89 289-18587
Fax +49 (0)89 289-18579

kilger@sec.in.tum.de
www.sec.in.tum.de

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 653 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 0/2][PR GDB/32956] gdb: fix GDB failing to find build-id debug files in linux mount namespaces

[Tom Tromey]

>>>>> "Fabian" == Fabian Kilger <kilger@sec.in.tum.de> writes:

Fabian> GDB can no longer find build-id debug files in docker containers.
Fabian> The cause is the new algorithm to look for a build-id-based debug file
Fabian> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba).
Fabian> It makes use of fileio_stat. As fileio_stat is not supported by
Fabian> gdb/nat/linux-namespace.c, all stat calls would be performed on the
Fabian> host and not inside the mount namespace. Furthermore, gdb/build-id.c
Fabian> calls target_fileio_stat with inferior none, thus stat will always be
Fabian> performed on the host mount namespace instead of the target's mount
Fabian> namespace.

Fabian> The patch is split into two parts: First, add support for fileio_stat
Fabian> querying the linux mount namespace and second, adjust gdb/build-id.c
Fabian> to call target_fileio_stat with current_inferior () instead of nullptr.

Fabian> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32956

Hi, I don't know anything at all about this area, but some keywords in
the thread triggered a memory of this patch:

https://inbox.sourceware.org/gdb-patches/20230321120126.1418012-1-benjamin@sipsolutions.net/

Can you say how it compares and whether your patch fixes that same
problem?

thanks,
Tom

Re: [PATCH 2/2][PR GDB/32956] gdb: query inferior's filesystem for build-id debug files

[Andrew Burgess]

Fabian Kilger <kilger@sec.in.tum.de> writes:

> Thanks for approving, I'll add the link to the commit message.
>
> If I merge this ahead of #1, can I leave it out in the v2 patch
> submission, or should it be included for completeness' sake - not sure
> what is desired.

Sorry, have been side tracked on other tasks.

I think you should merge this, and any new iteration would not include
this patch as this would now be part of GDB.

Thanks,
Andrew



>
> Best,
> Fabian
>
> On 5/23/25 20:20, Andrew Burgess wrote:
>> Fabian Kilger <kilger@sec.in.tum.de> writes:
>> 
>>> This fixes a bug related to build-id files with linux namespaces.
>>> Specifically, we expect the debug files to be present inside the container,
>>> thus the container filesystem should be queried if the program is running
>>> inside one.
>> 
>> You should add a bug link to the end of this commit message, like:
>> 
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32956
>> 
>> With that added, for this patch:
>> 
>> Approved-By: Andrew Burgess <aburgess@redhat.com>
>> 
>> You are welcome to merge this ahead of patch #1 if you want, as they are
>> related, but don't depend on each other.
>> 
>> Thanks,
>> Andrew
>> 
>> 
>> 
>>>
>>> ---
>>>  gdb/build-id.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdb/build-id.c b/gdb/build-id.c
>>> index 43a80dd3978..abbd65a7c12 100644
>>> --- a/gdb/build-id.c
>>> +++ b/gdb/build-id.c
>>> @@ -29,6 +29,7 @@
>>>  #include "gdbsupport/scoped_fd.h"
>>>  #include "debuginfod-support.h"
>>>  #include "extension.h"
>>> +#include "inferior.h"
>>>  
>>>  /* See build-id.h.  */
>>>  
>>> @@ -128,7 +129,8 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
>>>  	  if (supports_target_stat != TRIBOOL_FALSE)
>>>  	    {
>>>  	      struct stat sb;
>>> -	      int res = target_fileio_stat (nullptr, link_on_target, &sb,
>>> +	      int res = target_fileio_stat (current_inferior (),
>>> +					    link_on_target, &sb,
>>>  					    &target_errno);
>>>  
>>>  	      if (res != 0 && target_errno != FILEIO_ENOSYS)
>>> @@ -157,7 +159,7 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
>>>  		 the path doesn't exist, but we just assume that anything
>>>  		 other than EINVAL indicates the path doesn't exist.  */
>>>  	      std::optional<std::string> link_target
>>> -		= target_fileio_readlink (nullptr, link_on_target,
>>> +		= target_fileio_readlink (current_inferior (), link_on_target,
>>>  					  &target_errno);
>>>  	      if (link_target.has_value ()
>>>  		  || target_errno == FILEIO_EINVAL)
>>> -- 
>>> 2.49.0
>> 
>
> -- 
> Fabian Kilger, M.Sc.
> Wissenschaftlicher Mitarbeiter
>
> Technische Universität München
> TUM School of Computation, Information and Technology
> Chair of IT Security
>
> Boltzmannstraße 3
> 85748 Garching (bei München)
>
> Tel. +49 (0)89 289-18587
> Fax +49 (0)89 289-18579
>
> kilger@sec.in.tum.de
> www.sec.in.tum.de
> -----BEGIN PGP PUBLIC KEY BLOCK-----
>
> xjMEYHltfxYJKwYBBAHaRw8BAQdA7mzpLUfZIcIiMjdx+GBa8RuqZdMp/MUEpu4P
> DTb2YwXNJEZhYmlhbiBLaWxnZXIgPGtpbGdlckBzZWMuaW4udHVtLmRlPsKLBBMW
> CAAzFiEETPRi+vRLaNymGJvYr2lqRpshfmkFAmB5bX8CGwMFCwkIBwIGFQgJCgsC
> BRYCAwEAAAoJEK9pakabIX5pCzcA/ivCFRRbxJfpiwOzV5CvflcHPNN2LmCxSBlc
> rBpliBhWAP43PcAtWheftijoLpcwy3nD0TVTDRrJY/hRkKDbvmrWCM44BGB5bX8S
> CisGAQQBl1UBBQEBB0BtYlZed2qkwQWmV+MaUhC78XgZI0ezLuU2nr8bocqXCAMB
> CAfCeAQYFggAIBYhBEz0Yvr0S2jcphib2K9pakabIX5pBQJgeW1/AhsMAAoJEK9p
> akabIX5pUNQA/juajzwCYdtbo+sXQUlZufPiPwLiPr6LuJBNZwL6OlbmAQDvyu6h
> +X9K2gzgLviiNEmcCAddwynvjXiLt3c+oir7AA==
> =VdeZ
> -----END PGP PUBLIC KEY BLOCK-----

Re: [PATCH 0/2][PR GDB/32956] gdb: fix GDB failing to find build-id debug files in linux mount namespaces

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Fabian" == Fabian Kilger <kilger@sec.in.tum.de> writes:
>
> Fabian> GDB can no longer find build-id debug files in docker containers.
> Fabian> The cause is the new algorithm to look for a build-id-based debug file
> Fabian> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba).
> Fabian> It makes use of fileio_stat. As fileio_stat is not supported by
> Fabian> gdb/nat/linux-namespace.c, all stat calls would be performed on the
> Fabian> host and not inside the mount namespace. Furthermore, gdb/build-id.c
> Fabian> calls target_fileio_stat with inferior none, thus stat will always be
> Fabian> performed on the host mount namespace instead of the target's mount
> Fabian> namespace.
>
> Fabian> The patch is split into two parts: First, add support for fileio_stat
> Fabian> querying the linux mount namespace and second, adjust gdb/build-id.c
> Fabian> to call target_fileio_stat with current_inferior () instead of nullptr.
>
> Fabian> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32956
>
> Hi, I don't know anything at all about this area, but some keywords in
> the thread triggered a memory of this patch:
>
> https://inbox.sourceware.org/gdb-patches/20230321120126.1418012-1-benjamin@sipsolutions.net/
>
> Can you say how it compares and whether your patch fixes that same
> problem?

Tom,

I don't think this series is addressing the same issue as the patch
above.  I tested the issue from the above patch with this series, and
it's not fixed, and looking through the change, I don't think they're
the same problem.

However, after looking at the patch you linked, I figured writing a test
would be pretty easy, so I cleaned the patch up, added a test, and
posted it here:

  https://inbox.sourceware.org/gdb-patches/bc0e9b34f695afb0e14fd2fa1301bd0e44dd7b9b.1749574641.git.aburgess@redhat.com

Thanks,
Andrew

Re: [PATCH 2/2][PR GDB/32956] gdb: query inferior's filesystem for build-id debug files

[Fabian Kilger]

[-- Attachment #1.1.1: Type: text/plain, Size: 4948 bytes --]

No problem, thanks for getting back to me. As a first-time contributor
I'm a bit confused about the rules regarding merging and pushing.
According to gdb/MAINTAINERS, I should not have write access and need to
be first added, correct? And according to
  https://sourceware.org/gdb/wiki/ContributionChecklist
a moderator or maintainer needs to send me a form regarding FSF
copyright assignment first.

I would appreciate some clarifications/instructions on what to do next.

I've also prepared a PATCH v2 for the remaining changes, but wasn't sure
about some style points I asked about in:


https://inbox.sourceware.org/gdb-patches/06881879-1236-4ad7-b983-c195c1c0a84a@sec.in.tum.de/

Thanks,
Fabian


On 6/10/25 11:10, Andrew Burgess wrote:
> Fabian Kilger <kilger@sec.in.tum.de> writes:
> 
>> Thanks for approving, I'll add the link to the commit message.
>>
>> If I merge this ahead of #1, can I leave it out in the v2 patch
>> submission, or should it be included for completeness' sake - not sure
>> what is desired.
> 
> Sorry, have been side tracked on other tasks.
> 
> I think you should merge this, and any new iteration would not include
> this patch as this would now be part of GDB.
> 
> Thanks,
> Andrew
> 
> 
> 
>>
>> Best,
>> Fabian
>>
>> On 5/23/25 20:20, Andrew Burgess wrote:
>>> Fabian Kilger <kilger@sec.in.tum.de> writes:
>>>
>>>> This fixes a bug related to build-id files with linux namespaces.
>>>> Specifically, we expect the debug files to be present inside the container,
>>>> thus the container filesystem should be queried if the program is running
>>>> inside one.
>>>
>>> You should add a bug link to the end of this commit message, like:
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32956
>>>
>>> With that added, for this patch:
>>>
>>> Approved-By: Andrew Burgess <aburgess@redhat.com>
>>>
>>> You are welcome to merge this ahead of patch #1 if you want, as they are
>>> related, but don't depend on each other.
>>>
>>> Thanks,
>>> Andrew
>>>
>>>
>>>
>>>>
>>>> ---
>>>>  gdb/build-id.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gdb/build-id.c b/gdb/build-id.c
>>>> index 43a80dd3978..abbd65a7c12 100644
>>>> --- a/gdb/build-id.c
>>>> +++ b/gdb/build-id.c
>>>> @@ -29,6 +29,7 @@
>>>>  #include "gdbsupport/scoped_fd.h"
>>>>  #include "debuginfod-support.h"
>>>>  #include "extension.h"
>>>> +#include "inferior.h"
>>>>  
>>>>  /* See build-id.h.  */
>>>>  
>>>> @@ -128,7 +129,8 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
>>>>  	  if (supports_target_stat != TRIBOOL_FALSE)
>>>>  	    {
>>>>  	      struct stat sb;
>>>> -	      int res = target_fileio_stat (nullptr, link_on_target, &sb,
>>>> +	      int res = target_fileio_stat (current_inferior (),
>>>> +					    link_on_target, &sb,
>>>>  					    &target_errno);
>>>>  
>>>>  	      if (res != 0 && target_errno != FILEIO_ENOSYS)
>>>> @@ -157,7 +159,7 @@ build_id_to_debug_bfd_1 (const std::string &original_link,
>>>>  		 the path doesn't exist, but we just assume that anything
>>>>  		 other than EINVAL indicates the path doesn't exist.  */
>>>>  	      std::optional<std::string> link_target
>>>> -		= target_fileio_readlink (nullptr, link_on_target,
>>>> +		= target_fileio_readlink (current_inferior (), link_on_target,
>>>>  					  &target_errno);
>>>>  	      if (link_target.has_value ()
>>>>  		  || target_errno == FILEIO_EINVAL)
>>>> -- 
>>>> 2.49.0
>>>
>>
>> -- 
>> Fabian Kilger, M.Sc.
>> Wissenschaftlicher Mitarbeiter
>>
>> Technische Universität München
>> TUM School of Computation, Information and Technology
>> Chair of IT Security
>>
>> Boltzmannstraße 3
>> 85748 Garching (bei München)
>>
>> Tel. +49 (0)89 289-18587
>> Fax +49 (0)89 289-18579
>>
>> kilger@sec.in.tum.de
>> www.sec.in.tum.de
>> -----BEGIN PGP PUBLIC KEY BLOCK-----
>>
>> xjMEYHltfxYJKwYBBAHaRw8BAQdA7mzpLUfZIcIiMjdx+GBa8RuqZdMp/MUEpu4P
>> DTb2YwXNJEZhYmlhbiBLaWxnZXIgPGtpbGdlckBzZWMuaW4udHVtLmRlPsKLBBMW
>> CAAzFiEETPRi+vRLaNymGJvYr2lqRpshfmkFAmB5bX8CGwMFCwkIBwIGFQgJCgsC
>> BRYCAwEAAAoJEK9pakabIX5pCzcA/ivCFRRbxJfpiwOzV5CvflcHPNN2LmCxSBlc
>> rBpliBhWAP43PcAtWheftijoLpcwy3nD0TVTDRrJY/hRkKDbvmrWCM44BGB5bX8S
>> CisGAQQBl1UBBQEBB0BtYlZed2qkwQWmV+MaUhC78XgZI0ezLuU2nr8bocqXCAMB
>> CAfCeAQYFggAIBYhBEz0Yvr0S2jcphib2K9pakabIX5pBQJgeW1/AhsMAAoJEK9p
>> akabIX5pUNQA/juajzwCYdtbo+sXQUlZufPiPwLiPr6LuJBNZwL6OlbmAQDvyu6h
>> +X9K2gzgLviiNEmcCAddwynvjXiLt3c+oir7AA==
>> =VdeZ
>> -----END PGP PUBLIC KEY BLOCK-----
> 

-- 
Fabian Kilger, M.Sc.
Wissenschaftlicher Mitarbeiter

Technische Universität München
TUM School of Computation, Information and Technology
Chair of IT Security

Boltzmannstraße 3
Raum 01.08.053
85748 Garching (bei München)

Tel. +49 (0)89 289-18587
Fax +49 (0)89 289-18579

kilger@sec.in.tum.de
www.sec.in.tum.de

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 653 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 2/2][PR GDB/32956] gdb: query inferior's filesystem for build-id debug files

[Andrew Burgess]

Fabian Kilger <kilger@sec.in.tum.de> writes:

> No problem, thanks for getting back to me. As a first-time contributor
> I'm a bit confused about the rules regarding merging and pushing.
> According to gdb/MAINTAINERS, I should not have write access and need to
> be first added, correct? And according to
>   https://sourceware.org/gdb/wiki/ContributionChecklist
> a moderator or maintainer needs to send me a form regarding FSF
> copyright assignment first.
>
> I would appreciate some clarifications/instructions on what to do
> next.

Unless those forms are stored somewhere that I'm not aware of, then I
don't believe I have access to them, hopefully a maintainer that does
can share them with you.

Failing that, this page:

  https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html

suggests you could just email assign@gnu.org explain what projects you
wish to contribute too, and they will send you back the required
(e-)papers to fill in.

Before then, I could push this second patch for you, if you're happy for
me to do that.

>
> I've also prepared a PATCH v2 for the remaining changes, but wasn't sure
> about some style points I asked about in:
>
>
> https://inbox.sourceware.org/gdb-patches/06881879-1236-4ad7-b983-c195c1c0a84a@sec.in.tum.de/

I'll take a read through, and follow up there.

Thanks,
Andrew

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Andrew Burgess]

Fabian Kilger <kilger@sec.in.tum.de> writes:

> Thanks for reviewing, I'll be including these in v2. I have questions
> regarding some style recommendations, where I feel like I'd be breaking
> the style of the remaining file.
>
> On 5/23/25 20:14, Andrew Burgess wrote:
>> Fabian Kilger <kilger@sec.in.tum.de> writes:
>> 
>> Thanks for working on this.  This mostly looks good, I have just a few
>> style issues that I think should be fixed, but it's all pretty minor.
>> 
>> 
>>> The new algorithm to look for a build-id-based debug file
>>> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
>>> makes use of fileio_stat. As fileio_stat was not supported by
>>> linux-namespace.c, all stat calls would be performed on the host
>>> and not inside the namespace
>>>
>>> ---
>>>  gdb/linux-nat.c            | 14 ++++++++
>>>  gdb/linux-nat.h            |  3 ++
>>>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>>>  gdb/nat/linux-namespaces.h |  6 ++++
>>>  4 files changed, 94 insertions(+)
>>>
>>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>>> index 3f252370c7b..478a7977c4d 100644
>>> --- a/gdb/linux-nat.c
>>> +++ b/gdb/linux-nat.c
>>> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>>>    return fd;
>>>  }
>>>  
>>> +/* Implementation of to_fileio_stat.  */
>>> +int
>> 
>> Add an empty line after the comment please to match the other functions
>> in this file.
>> 
>>> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
>>> +			       struct stat *sb, fileio_error *target_errno)
>>> +{
>>> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
>>> +			    filename, sb);
>> 
>> There's no need to wrap this argument list.
>> 
>
> I've tried copying the style of the other functions in the file. All
> fileio_X functions wrap the argument list after linux_nat_fileio_pid_of.
> Specifically also fileio_unlink, which has even less arguments. As such,
> I feel like not wrapping would break with the style of the other
> functions. Should I still not wrap it?

My understanding is that the "correct" style takes precedent over
matching "incorrect" style in a file.  The hope is that, over time, as
people touch the code, instances of incorrect style will slowly be
fixed.  But if we keep adding more incorrect style to match existing
code, then things will never get better.

Worse, due to copy&paste, the more incorrectly styled code we have, the
more the incorrect styling spreads from file to file.

>
>>> +
>>> +  if (r == -1)
>>> +    *target_errno = host_to_fileio_error (errno);
>>> +
>>> +  return r;
>>> +}
>>> +
>>>  /* Implementation of to_fileio_readlink.  */
>>>  
>>>  std::optional<std::string>
>>> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
>>> index b630b858e34..42d1ec142b3 100644
>>> --- a/gdb/linux-nat.h
>>> +++ b/gdb/linux-nat.h
>>> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>>>  		     const char *filename,
>>>  		     fileio_error *target_errno) override;
>>>  
>>> +  int fileio_stat (struct inferior *inf, const char *filename,
>>> +		   struct stat *sb, fileio_error *target_errno) override;
>>> +
>>>    int fileio_unlink (struct inferior *inf,
>>>  		     const char *filename,
>>>  		     fileio_error *target_errno) override;
>>> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
>>> index 19a05eec905..aa74e9df950 100644
>>> --- a/gdb/nat/linux-namespaces.c
>>> +++ b/gdb/nat/linux-namespaces.c
>>> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>>>         MNSH_RET_INT.  */
>>>      MNSH_REQ_SETNS,
>>>  
>>> +    /* A request that the helper call stat.  The single
>>> +       argument (the filename) should be passed in BUF, and
>>> +       should include a terminating NUL character.  The helper
>>> +       should respond with a MNSH_RET_INTSTR.  */
>>> +    MNSH_REQ_STAT,
>>> +
>>>      /* A request that the helper call open.  Arguments should
>>>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>>>         should include a terminating NUL character.  The helper
>>> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>>>        debug_printf ("ERROR");
>>>        break;
>>>  
>>> +    case MNSH_REQ_STAT:
>>> +      debug_printf ("STAT");
>>> +      break;
>>> +
>>>      case MNSH_REQ_SETNS:
>>>        debug_printf ("SETNS");
>>>        break;
>>> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>>>    return mnsh_return_int (sock, result, errno);
>>>  }
>>>  
>>> +
>>> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
>>> +
>>> +static ssize_t
>>> +mnsh_handle_stat(int sock, const char *filename)
>>> +{
>>> +  struct stat sb;
>>> +  int stat_ok = stat(filename, &sb);
>> 
>> Space needed after 'stat'.
>> 
>>> +
>>> +  return mnsh_return_intstr(sock, stat_ok, &sb,
>>> +			    stat_ok == -1 ? 0 : sizeof (sb),
>>> +			    errno);
>> 
>> Space needed after 'mnsh_return_intstr'.
>> 
>>> +}
>>> +
>>>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>>>  
>>>  static ssize_t
>>> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>>>  		response = mnsh_handle_setns (sock, fd, int1);
>>>  	      break;
>>>  
>>> +	    case MNSH_REQ_STAT:
>>> +	      if (size > 0 && buf[size - 1] == '\0')
>>> +		response = mnsh_handle_stat(sock, buf);
>> 
>> Space needed after 'mnsh_handle_stat'
>> 
>>> +	      break;
>>> +
>>>  	    case MNSH_REQ_OPEN:
>>>  	      if (size > 0 && buf[size - 1] == '\0')
>>>  		response = mnsh_handle_open (sock, buf, int1, int2);
>>> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>>>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>>>  		     filename, strlen (filename) + 1)
>>>  
>>> +#define mnsh_send_stat(helper, filename) \
>>> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
>>> +		     filename, strlen (filename) + 1)
>>> +
>>>  #define mnsh_send_unlink(helper, filename) \
>>>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>>>  		     filename, strlen (filename) + 1)
>>> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>>>    return MNSH_FS_HELPER;
>>>  }
>>>  
>>> +
>>> +/* See nat/linux-namespaces.h.  */
>>> +int
>> 
>> Move the empty line from before the comment to after the comment please.
>> 
>>> +linux_mntns_stat (pid_t pid, const char *filename,
>>> +		  struct stat *sb)
>>> +{
>>> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
>>> +  struct linux_mnsh *helper;
>>> +  int stat_ok, error;
>>> +  ssize_t size;
>> 
>> The declarations should be moved inline below.
>
> Here I've also copied the style of the remaining functions: All
> functions in that file declare the variables at the beginning in the
> exact same fashion. See linux_mntns_open_cloexec right below it, for
> example.

As above.  Much of this code originated from when GDB was a C project,
at which time the GDB style was that everything should be declared at
the start of the scope, even when C itself no longer required that.

Shortly after the switch to C++ the style changed so that things should
be declared as close to the first use as possible.


You're right to check these things.  And it can be weird to go against
the file's existing style, but hopefully, over time, GDB will become
more consistent.

Thanks,
Andrew

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Andrew Burgess]

Fabian Kilger <kilger@sec.in.tum.de> writes:

> While looking at it, I've noticed all implementations of stat functions
> actually use lstat and not stat. This maybe should be modified in the
> namespace case as well and I'll be changing this for v2. However, I
> could not directly find a rationale behind why *_stat functions call
> lstat instead. Possibly, it might make sense renaming the
> target_fileio_stat to target_fileio_lstat as well, though this would be
> an independent change.

Without digging into the history I say for sure why it's lstat over
stat.

I agree with you that renaming the API might make some sense, though I
suspect we're really a little stuck, the remote protocol packet is
called 'stat', which is unfortunate, we cannot really change that.

I do agree that within the namespace code we should probably use lstat
in order to be consistent.

Thanks,
Andrew



>
> Best,
> Fabian
>
> On 5/24/25 21:43, Fabian Kilger wrote:
>> Hi Andrew,
>> 
>> you're right, I'm directly using GDB and wasn't aware of that gdbserver
>> module. I'll be adding it to the patch for v2.
>> 
>> Best,
>> Fabian
>> 
>> On 5/24/25 12:46, Andrew Burgess wrote:
>>> Fabian Kilger <kilger@sec.in.tum.de> writes:
>>>
>>>> The new algorithm to look for a build-id-based debug file
>>>> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
>>>> makes use of fileio_stat. As fileio_stat was not supported by
>>>> linux-namespace.c, all stat calls would be performed on the host
>>>> and not inside the namespace
>>>>
>>>> ---
>>>>  gdb/linux-nat.c            | 14 ++++++++
>>>>  gdb/linux-nat.h            |  3 ++
>>>>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>>>>  gdb/nat/linux-namespaces.h |  6 ++++
>>>
>>> Thinking about this some more, I realised that this is missing the
>>> gdbserver related changes.
>>>
>>> If you search in the gdbserver/ directory for how multifs_readlink is
>>> used then this will give a pretty good idea for how a new multifs_stat
>>> should be added.
>>>
>>> For manual testing things will be pretty similar to testing GDB.  On the
>>> same host as your container, but outside of the container, start
>>> gdbserver:
>>>
>>>   $ gdbserver --multi --once :54321
>>>
>>> Then on the same host, start GDB, and within GDB:
>>>
>>>   (gdb) target extended-remote :54321
>>>   (gdb) attach PID
>>>
>>> I'm assuming that currently you are just attaching directly from GDB to
>>> a process within the container?
>>>
>>> Thanks,
>>> Andrew
>>>
>>>>  4 files changed, 94 insertions(+)
>>>>
>>>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>>>> index 3f252370c7b..478a7977c4d 100644
>>>> --- a/gdb/linux-nat.c
>>>> +++ b/gdb/linux-nat.c
>>>> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>>>>    return fd;
>>>>  }
>>>>  
>>>> +/* Implementation of to_fileio_stat.  */
>>>> +int
>>>> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
>>>> +			       struct stat *sb, fileio_error *target_errno)
>>>> +{
>>>> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
>>>> +			    filename, sb);
>>>> +
>>>> +  if (r == -1)
>>>> +    *target_errno = host_to_fileio_error (errno);
>>>> +
>>>> +  return r;
>>>> +}
>>>> +
>>>>  /* Implementation of to_fileio_readlink.  */
>>>>  
>>>>  std::optional<std::string>
>>>> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
>>>> index b630b858e34..42d1ec142b3 100644
>>>> --- a/gdb/linux-nat.h
>>>> +++ b/gdb/linux-nat.h
>>>> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>>>>  		     const char *filename,
>>>>  		     fileio_error *target_errno) override;
>>>>  
>>>> +  int fileio_stat (struct inferior *inf, const char *filename,
>>>> +		   struct stat *sb, fileio_error *target_errno) override;
>>>> +
>>>>    int fileio_unlink (struct inferior *inf,
>>>>  		     const char *filename,
>>>>  		     fileio_error *target_errno) override;
>>>> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
>>>> index 19a05eec905..aa74e9df950 100644
>>>> --- a/gdb/nat/linux-namespaces.c
>>>> +++ b/gdb/nat/linux-namespaces.c
>>>> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>>>>         MNSH_RET_INT.  */
>>>>      MNSH_REQ_SETNS,
>>>>  
>>>> +    /* A request that the helper call stat.  The single
>>>> +       argument (the filename) should be passed in BUF, and
>>>> +       should include a terminating NUL character.  The helper
>>>> +       should respond with a MNSH_RET_INTSTR.  */
>>>> +    MNSH_REQ_STAT,
>>>> +
>>>>      /* A request that the helper call open.  Arguments should
>>>>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>>>>         should include a terminating NUL character.  The helper
>>>> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>>>>        debug_printf ("ERROR");
>>>>        break;
>>>>  
>>>> +    case MNSH_REQ_STAT:
>>>> +      debug_printf ("STAT");
>>>> +      break;
>>>> +
>>>>      case MNSH_REQ_SETNS:
>>>>        debug_printf ("SETNS");
>>>>        break;
>>>> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>>>>    return mnsh_return_int (sock, result, errno);
>>>>  }
>>>>  
>>>> +
>>>> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
>>>> +
>>>> +static ssize_t
>>>> +mnsh_handle_stat(int sock, const char *filename)
>>>> +{
>>>> +  struct stat sb;
>>>> +  int stat_ok = stat(filename, &sb);
>>>> +
>>>> +  return mnsh_return_intstr(sock, stat_ok, &sb,
>>>> +			    stat_ok == -1 ? 0 : sizeof (sb),
>>>> +			    errno);
>>>> +}
>>>> +
>>>>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>>>>  
>>>>  static ssize_t
>>>> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>>>>  		response = mnsh_handle_setns (sock, fd, int1);
>>>>  	      break;
>>>>  
>>>> +	    case MNSH_REQ_STAT:
>>>> +	      if (size > 0 && buf[size - 1] == '\0')
>>>> +		response = mnsh_handle_stat(sock, buf);
>>>> +	      break;
>>>> +
>>>>  	    case MNSH_REQ_OPEN:
>>>>  	      if (size > 0 && buf[size - 1] == '\0')
>>>>  		response = mnsh_handle_open (sock, buf, int1, int2);
>>>> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>>>>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>>>>  		     filename, strlen (filename) + 1)
>>>>  
>>>> +#define mnsh_send_stat(helper, filename) \
>>>> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
>>>> +		     filename, strlen (filename) + 1)
>>>> +
>>>>  #define mnsh_send_unlink(helper, filename) \
>>>>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>>>>  		     filename, strlen (filename) + 1)
>>>> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>>>>    return MNSH_FS_HELPER;
>>>>  }
>>>>  
>>>> +
>>>> +/* See nat/linux-namespaces.h.  */
>>>> +int
>>>> +linux_mntns_stat (pid_t pid, const char *filename,
>>>> +		  struct stat *sb)
>>>> +{
>>>> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
>>>> +  struct linux_mnsh *helper;
>>>> +  int stat_ok, error;
>>>> +  ssize_t size;
>>>> +
>>>> +  if (access == MNSH_FS_ERROR)
>>>> +    return -1;
>>>> +
>>>> +  if (access == MNSH_FS_DIRECT)
>>>> +    return stat(filename, sb);
>>>> +
>>>> +  gdb_assert (access == MNSH_FS_HELPER);
>>>> +
>>>> +  helper = linux_mntns_get_helper ();
>>>> +
>>>> +  size = mnsh_send_stat (helper, filename);
>>>> +  if (size < 0)
>>>> +    return -1;
>>>> +
>>>> +  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
>>>> +
>>>> +  if (size < 0)
>>>> +    {
>>>> +      stat_ok = -1;
>>>> +      errno = error;
>>>> +    }
>>>> +  else
>>>> +    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
>>>> +
>>>> +  return stat_ok;
>>>> +}
>>>> +
>>>>  /* See nat/linux-namespaces.h.  */
>>>>  
>>>>  int
>>>> diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
>>>> index 4327292950b..825cb27eb2d 100644
>>>> --- a/gdb/nat/linux-namespaces.h
>>>> +++ b/gdb/nat/linux-namespaces.h
>>>> @@ -58,6 +58,12 @@ enum linux_ns_type
>>>>  
>>>>  extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
>>>>  
>>>> +/* Like stat(2), but in the mount namespace of process
>>>> +   PID.  */
>>>> +
>>>> +extern int linux_mntns_stat (pid_t pid, const char *filename,
>>>> +			     struct stat *sb);
>>>> +
>>>>  /* Like gdb_open_cloexec, but in the mount namespace of process
>>>>     PID.  */
>>>>  
>>>> -- 
>>>> 2.49.0
>>>
>> 
>
> -- 
> Fabian Kilger, M.Sc.
> Wissenschaftlicher Mitarbeiter
>
> Technische Universität München
> TUM School of Computation, Information and Technology
> Chair of IT Security
>
> Boltzmannstraße 3
> 85748 Garching (bei München)
>
> Tel. +49 (0)89 289-18587
> Fax +49 (0)89 289-18579
>
> kilger@sec.in.tum.de
> www.sec.in.tum.de
> -----BEGIN PGP PUBLIC KEY BLOCK-----
>
> xjMEYHltfxYJKwYBBAHaRw8BAQdA7mzpLUfZIcIiMjdx+GBa8RuqZdMp/MUEpu4P
> DTb2YwXNJEZhYmlhbiBLaWxnZXIgPGtpbGdlckBzZWMuaW4udHVtLmRlPsKLBBMW
> CAAzFiEETPRi+vRLaNymGJvYr2lqRpshfmkFAmB5bX8CGwMFCwkIBwIGFQgJCgsC
> BRYCAwEAAAoJEK9pakabIX5pCzcA/ivCFRRbxJfpiwOzV5CvflcHPNN2LmCxSBlc
> rBpliBhWAP43PcAtWheftijoLpcwy3nD0TVTDRrJY/hRkKDbvmrWCM44BGB5bX8S
> CisGAQQBl1UBBQEBB0BtYlZed2qkwQWmV+MaUhC78XgZI0ezLuU2nr8bocqXCAMB
> CAfCeAQYFggAIBYhBEz0Yvr0S2jcphib2K9pakabIX5pBQJgeW1/AhsMAAoJEK9p
> akabIX5pUNQA/juajzwCYdtbo+sXQUlZufPiPwLiPr6LuJBNZwL6OlbmAQDvyu6h
> +X9K2gzgLviiNEmcCAddwynvjXiLt3c+oir7AA==
> =VdeZ
> -----END PGP PUBLIC KEY BLOCK-----

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> Fabian Kilger <kilger@sec.in.tum.de> writes:
>
>> While looking at it, I've noticed all implementations of stat functions
>> actually use lstat and not stat. This maybe should be modified in the
>> namespace case as well and I'll be changing this for v2. However, I
>> could not directly find a rationale behind why *_stat functions call
>> lstat instead. Possibly, it might make sense renaming the
>> target_fileio_stat to target_fileio_lstat as well, though this would be
>> an independent change.
>
> Without digging into the history I say for sure why it's lstat over
> stat.
>
> I agree with you that renaming the API might make some sense, though I
> suspect we're really a little stuck, the remote protocol packet is
> called 'stat', which is unfortunate, we cannot really change that.

OK, so I dug into the history, and it would appear that (a) it was just
a mistake, and (b) it was all my fault :-/

The code in question does want lstat, so really the whole API should
have been called lstat.

Most of this can easily be fixed, but the remote protocol part we're
unfortunately stuck with.  Though I wonder if we can "fix" this by just
have a 'stat' and 'lstat' remote protocol packet, change 'stat' to do an
actual 'stat', then on the GDB side we'll change the internal target API
to be called 'lstat'.  This would effectively orphan the remote 'stat'
packet, as nobody would use it, but that's OK I think, and only minimal
overhead.  I'll take a look at rolling something like this together.

For you, right now, you should name your functions 'stat', but do an
'lstat' to be consistent.

Thanks,
Andrew

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> OK, so I dug into the history, and it would appear that (a) it was just
Andrew> a mistake, and (b) it was all my fault :-/

A classic programming story, I can't tell you how many times I've been
there.

Andrew> Most of this can easily be fixed, but the remote protocol part we're
Andrew> unfortunately stuck with.  Though I wonder if we can "fix" this by just
Andrew> have a 'stat' and 'lstat' remote protocol packet, change 'stat' to do an
Andrew> actual 'stat', then on the GDB side we'll change the internal target API
Andrew> to be called 'lstat'.  This would effectively orphan the remote 'stat'
Andrew> packet, as nobody would use it, but that's OK I think, and only minimal
Andrew> overhead.  I'll take a look at rolling something like this together.

Wouldn't this negatively affect any existing remotes that implement stat-as-lstat?
(I don't know if any of these actually exist.)

It might be better to just document it.

Tom

Re: [PATCH 2/2][PR GDB/32956] gdb: query inferior's filesystem for build-id debug files

[Simon Marchi]

On 6/11/25 5:35 AM, Andrew Burgess wrote:
> Fabian Kilger <kilger@sec.in.tum.de> writes:
> 
>> No problem, thanks for getting back to me. As a first-time contributor
>> I'm a bit confused about the rules regarding merging and pushing.
>> According to gdb/MAINTAINERS, I should not have write access and need to
>> be first added, correct? And according to
>>   https://sourceware.org/gdb/wiki/ContributionChecklist
>> a moderator or maintainer needs to send me a form regarding FSF
>> copyright assignment first.
>>
>> I would appreciate some clarifications/instructions on what to do
>> next.
> 
> Unless those forms are stored somewhere that I'm not aware of, then I
> don't believe I have access to them, hopefully a maintainer that does
> can share them with you.

I usually just link to the gnulib repo here (don't know why they are
stored there, but they are):

https://gitweb.git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=doc/Copyright/request-assign.future;hb=HEAD

It contains instructions on what to do, so the contributor can send it
directly to the FSF.

Simon

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> OK, so I dug into the history, and it would appear that (a) it was just
> Andrew> a mistake, and (b) it was all my fault :-/
>
> A classic programming story, I can't tell you how many times I've been
> there.
>
> Andrew> Most of this can easily be fixed, but the remote protocol part we're
> Andrew> unfortunately stuck with.  Though I wonder if we can "fix" this by just
> Andrew> have a 'stat' and 'lstat' remote protocol packet, change 'stat' to do an
> Andrew> actual 'stat', then on the GDB side we'll change the internal target API
> Andrew> to be called 'lstat'.  This would effectively orphan the remote 'stat'
> Andrew> packet, as nobody would use it, but that's OK I think, and only minimal
> Andrew> overhead.  I'll take a look at rolling something like this together.
>
> Wouldn't this negatively affect any existing remotes that implement stat-as-lstat?
> (I don't know if any of these actually exist.)

Yes, you are correct.  But:

  - The vFile:stat documentation is pretty vague, it just says: get
    information about a file.  So if there are other remotes that
    implement this then they probably implemented 'stat' anyway as that
    would be the logical first choice.

    I would propose, as part of this change, to better document what is
    expected from 'stat' and 'lstat'.

    It could (maybe) be argued that the current 'lstat' implementation
    was a bug, and it should have been 'stat'.

  - The vFile:stat packet was only added in GDB 16, and there's only the
    one use of it from GDB.  In most cases implementing vFile:stat as an
    actual 'stat' will go unnoticed, though the GDB testsuite does rely
    on it being an actual 'lstat', so if someone is testing a custom
    remote against the GDB testsuite they might have figured out that
    they needed lstat instead of stat.

    My hope would be that adding a new vFile:lstat packet, and noting in
    the NEWS file that the vFile:stat should _really_ be 'stat' would
    allow any other remotes to course correct.

If you feel strongly though then we can for sure just leave the
vFile:stat packet implemented as 'lstat' and document it as such.

Thanks,
Andrew


>
> It might be better to just document it.
>
> Tom

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Fabian Kilger]

[-- Attachment #1.1.1: Type: text/plain, Size: 2848 bytes --]


On 6/11/25 11:58, Andrew Burgess wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
> 
>> Fabian Kilger <kilger@sec.in.tum.de> writes:
>>
>>> While looking at it, I've noticed all implementations of stat functions
>>> actually use lstat and not stat. This maybe should be modified in the
>>> namespace case as well and I'll be changing this for v2. However, I
>>> could not directly find a rationale behind why *_stat functions call
>>> lstat instead. Possibly, it might make sense renaming the
>>> target_fileio_stat to target_fileio_lstat as well, though this would be
>>> an independent change.
>>
>> Without digging into the history I say for sure why it's lstat over
>> stat.
>>
>> I agree with you that renaming the API might make some sense, though I
>> suspect we're really a little stuck, the remote protocol packet is
>> called 'stat', which is unfortunate, we cannot really change that.
> 
> OK, so I dug into the history, and it would appear that (a) it was just
> a mistake, and (b) it was all my fault :-/
> 
> The code in question does want lstat, so really the whole API should
> have been called lstat.
> 
> Most of this can easily be fixed, but the remote protocol part we're
> unfortunately stuck with.  Though I wonder if we can "fix" this by just
> have a 'stat' and 'lstat' remote protocol packet, change 'stat' to do an
> actual 'stat', then on the GDB side we'll change the internal target API
> to be called 'lstat'.  This would effectively orphan the remote 'stat'
> packet, as nobody would use it, but that's OK I think, and only minimal
> overhead.  I'll take a look at rolling something like this together.
> 
> For you, right now, you should name your functions 'stat', but do an
> 'lstat' to be consistent.
> 
> Thanks,
> Andrew
> 

Following the same logic of '"correct" style takes precedent over
matching "incorrect" style', would it not make sense to start using
lstat in the name for those functions that actually perform lstat? The
code would be overall more clear if the functions are called _lstat and
perform lstat imo. In case a 'lstat' remote protocol packet will be
added, it would also be already clear if the existing functions are
already named lstat. Especially in the namespace helper code and the
communication protocol this lstat/stat name confusion would spread in a
call chain of like 5 functions.

Maybe my point would be more clear if I submit the current version of
the patch.

Best,
Fabian


-- 
Fabian Kilger, M.Sc.
Wissenschaftlicher Mitarbeiter

Technische Universität München
TUM School of Computation, Information and Technology
Chair of IT Security

Boltzmannstraße 3
Raum 01.08.053
85748 Garching (bei München)

Tel. +49 (0)89 289-18587
Fax +49 (0)89 289-18579

kilger@sec.in.tum.de
www.sec.in.tum.de

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 653 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew>   - The vFile:stat packet was only added in GDB 16

... I didn't realize that.

Andrew> If you feel strongly though then we can for sure just leave the
Andrew> vFile:stat packet implemented as 'lstat' and document it as such.

Given the above it seems fine to go ahead

Tom