diff mbox series

[v2,5/6] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t

Message ID 20260825100912.514232-6-matthieu.longo@arm.com
State New
Headers
Series gdb: introduce file_reader_t to read procfs files |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply

Commit Message

Matthieu Longo Aug. 25, 2026, 10:09 a.m. UTC 
  The previous implementation of linux_find_memory_regions_full could
still return success even when none of the /proc/PID/[s]maps files
existed, or all reads returned 0 bytes (this last case can happen on
Linux when the thread-group leader has exited).
As a result, the function could incorrectly succeed, allowing core
dump generation via the gcore command.
This logical defect was allowing, by chance, the function to return
success and hence, allowing fortuitously the coredump generation via
gcore command (see gcore-stale-thread test for more details).

A previous patch in this patch series fixed this issue by using the
first LWP ID of the current inferior instead of relying on the PID.
This patch adapts the code to use file_reader_t and makes the function
return an error if reading any of the procfs files fails.

Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
---
 gdb/linux-tdep.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Comments

Andrew Burgess Sept. 11, 2026, 8:34 a.m. UTC | #1 
Matthieu Longo <matthieu.longo@arm.com> writes:

> The previous implementation of linux_find_memory_regions_full could
> still return success even when none of the /proc/PID/[s]maps files
> existed, or all reads returned 0 bytes (this last case can happen on
> Linux when the thread-group leader has exited).
> As a result, the function could incorrectly succeed, allowing core
> dump generation via the gcore command.
> This logical defect was allowing, by chance, the function to return
> success and hence, allowing fortuitously the coredump generation via
> gcore command (see gcore-stale-thread test for more details).
>
> A previous patch in this patch series fixed this issue by using the
> first LWP ID of the current inferior instead of relying on the PID.
> This patch adapts the code to use file_reader_t and makes the function
> return an error if reading any of the procfs files fails.

This commit message seems out of date.  As far as I can tell the issue
you are describing here isn't fixed yet.

Further, I took a look at gdb.threads/gcore-stale-thread.exp and
couldn't find any text talking about this issue, so saying "see
gcore-stale-thread test for more details" isn't super helpful.

It doesn't sound like this patch was actually aiming to fix the bug you
described anyway, just to convert to the file_reader_t class, so
honestly, I'd just drop all discussion of the previously fixed bug.

But if this patch is fixing the bug then I'd expect either a new test to
be added, or if gcore-stale-thread.exp does hit this bug, I would have
expected the test to be updated now that the bug is fixed.

Thanks,
Andrew

>
> Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
>  gdb/linux-tdep.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index e2c5b2d8815..2833d8a4bb6 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1828,24 +1828,22 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
>  	}
>      }
>  
> -  std::string maps_filename = string_printf ("/proc/%ld/smaps", ptid.lwp ());
> -
> -  gdb::unique_xmalloc_ptr<char> data
> -    = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
> +  std::vector<smaps_data> smaps;
>  
> -  if (data == NULL)
> +  file_reader_t<char> smaps_freader
> +    (string_printf ("/proc/%ld/smaps", ptid.lwp ()));
> +  if (smaps_freader)
> +    smaps = parse_smaps_data (smaps_freader);
> +  else
>      {
>        /* Older Linux kernels did not support /proc/PID/smaps.  */
> -      maps_filename = string_printf ("/proc/%ld/maps", ptid.lwp ());
> -      data = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
> -
> -      if (data == nullptr)
> +      file_reader_t<char> maps_freader
> +	(string_printf ("/proc/%ld/maps", ptid.lwp ()));
> +      if (!maps_freader)
>  	return false;
> +      smaps = parse_smaps_data (maps_freader);
>      }
>  
> -  /* Parse the contents of smaps into a vector.  */
> -  std::vector<smaps_data> smaps = parse_smaps_data (data.get (), maps_filename);
> -
>    for (const smaps_data &map: smaps)
>      {
>        /* Invoke the callback function to create the corefile segment.  */
> -- 
> 2.55.0
diff mbox series

Patch

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index e2c5b2d8815..2833d8a4bb6 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1828,24 +1828,22 @@  linux_find_memory_regions_full (struct gdbarch *gdbarch,
 	}
     }
 
-  std::string maps_filename = string_printf ("/proc/%ld/smaps", ptid.lwp ());
-
-  gdb::unique_xmalloc_ptr<char> data
-    = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
+  std::vector<smaps_data> smaps;
 
-  if (data == NULL)
+  file_reader_t<char> smaps_freader
+    (string_printf ("/proc/%ld/smaps", ptid.lwp ()));
+  if (smaps_freader)
+    smaps = parse_smaps_data (smaps_freader);
+  else
     {
       /* Older Linux kernels did not support /proc/PID/smaps.  */
-      maps_filename = string_printf ("/proc/%ld/maps", ptid.lwp ());
-      data = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
-
-      if (data == nullptr)
+      file_reader_t<char> maps_freader
+	(string_printf ("/proc/%ld/maps", ptid.lwp ()));
+      if (!maps_freader)
 	return false;
+      smaps = parse_smaps_data (maps_freader);
     }
 
-  /* Parse the contents of smaps into a vector.  */
-  std::vector<smaps_data> smaps = parse_smaps_data (data.get (), maps_filename);
-
   for (const smaps_data &map: smaps)
     {
       /* Invoke the callback function to create the corefile segment.  */