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

[Andrew Burgess <aburgess@redhat.com>]

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