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

[Andrew Burgess <aburgess@redhat.com>]

"brandon.belew" <brandon.belew@zetier.com> writes:

> 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:
>
>> 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).

I think you should reconsider here.  The function returns LONGEST, so
that's what should be used.  GDB's general policy is to fix little
bugs like this as the code gets touched for other reasons.

Otherwise, I agree with Luis, this looks great.  If you repost with the
description in the commit message we can get this merged.

Thanks,
Andrew

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