Re: [PATCH] [PR corefiles/32441] Fix segfault if target_fileio_read_alloc fails

[Luis Machado <luis.machado@arm.com>]

Hi Brandon,

Sorry for the late reply. Again, feel free to ping it if it takes over a week without
a reply.

On 12/20/24 22:17, brandon.belew wrote:
> Check for target_fileio_read_alloc failure in linux_fill_prpsinfo
> before dereferencing buffer. This fixes a segfault in the 'gcore'
> command when attached to certain remote targets.
> ---
> This is my first contribution to GDB, and my first use of
> git-send-email, so please let me know if this is formatted
> incorrectly! I initially submitted the bug and a v1 patch at
> https://sourceware.org/bugzilla/show_bug.cgi?id=32441 and received the
> following from Thiago Bauermann:


Formatting-wise, The commit message goes into the patch itself. See other examples
on the list.

Also, since you've opened a bug, we have hooks to refer to the bug. For instance,
for commit ca263aec20adfffe6f9dab3a18f8a7b24667f99c.

PR testsuite/32489
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32489

> 
>> Thank you for the patch. In general it looks good to me, just a couple of minor
>> comments:
>>
>> 1. Since target_fileio_read_alloc () returns LONGEST, I think it's better if
>> the buf_len variable also has that type.
> 
> I decided to stick with ssize_t for the variable, as this matches the
> usage elsewhere in linux-tdep.c in linux_info_proc (which already was
> correctly checking the length).
> 
>> 2. GDB is (very) slowly transitioning from C to C++. We currently prefer to use
>> nullptr rather than NULL, so I suggest using this patch as an opportunity to
>> change NULL to nullptr in lines 1876, 1877 and 1879.
> 
> I made the requested NULL -> nullptr changes.
> 
> Let me know if this is good or if I need to make any changes in my
> workflow to adhere to GNU or gdb project conventions.
> 
>  gdb/linux-tdep.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index d3452059ce2..c10c4c76451 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1867,17 +1867,17 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
>    /* The number of fields read by `sscanf'.  */
>    int n_fields = 0;
> 
> -  gdb_assert (p != NULL);
> +  gdb_assert (p != nullptr);
> 
>    /* Obtaining PID and filename.  */
>    pid = inferior_ptid.pid ();
>    xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
>    /* The full name of the program which generated the corefile.  */
> -  gdb_byte *buf = NULL;
> -  size_t buf_len = target_fileio_read_alloc (NULL, filename, &buf);
> +  gdb_byte *buf = nullptr;
> +  ssize_t buf_len = target_fileio_read_alloc (nullptr, filename, &buf);
>    gdb::unique_xmalloc_ptr<char> fname ((char *)buf);
> 
> -  if (buf_len < 1 || fname.get ()[0] == '\0')
> +  if (buf_len < 1 || fname.get () == nullptr || fname.get ()[0] == '\0')
>      {
>        /* No program name was read, so we won't be able to retrieve more
>  	 information about the process.  */
> --
> 2.46.0

The change itself looks OK to me. I'd like another pair of eyes to look at
it before we approve it. I think this can go in as a trivial change when it
gets approved.

Reviewed-By: Luis Machado <luis.machado@arm.com>