Re: [PATCH v2] [gdb] Fix segfault in sig_write

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> I ran into a segfault in sig_write:
> ...
>   if (gdb_stderr == nullptr || gdb_stderr->fd () == -1)
> ...
>
> [ A regression since commit 817003ed469 ("Rewrite output redirection and
> logging"). ]
>
> This happens as follows.
>
> First, we get gdb_stderr by calling current_gdb_stderr, which returns
> current_ui.current_interpreter.m_stderr.get (), which is not nullptr.
>
> Then we do "gdb_stderr->fd ()", which gets us to
> wrapped_file<ui::ui_file_ptr<&ui::m_ui_stderr> >::fd:
> ...
>   int fd () const override
>   { return m_stream->fd (); }
> ...
>
> The "m_stream->" part brings us to:
> ...
>   /* A "smart pointer" that references a particular member of the
>      current UI.  */
>   template<ui_file *ui::* F>
>   struct ui_file_ptr
>   {
>     ui_file *operator-> () const
>     {
>       return current_ui->*F;
>     }
>   };
> ...
> which does "current_ui.m_ui_stderr" which indeed is nullptr.

It would be nice if the commit message explained *why* there's a nullptr
there.

I followed the path back through the many iterations of this work to
what I think is the analysis of why this happens:

  https://inbox.sourceware.org/gdb-patches/716d68d7-6888-4da6-a0e1-d8bf6c75f663@suse.de

Which makes me wonder, is this still a problem after commit:

  commit b171f68e9450e3bba074b0fe918b78c2ebb03af8
  Author: Tom de Vries <tdevries@suse.de>
  Date:   Thu Apr 2 23:09:09 2026 +0200

    [gdb/tui] Make tui_setup_io more robust


>
> Fix this in wrapped_file::fd by checking for a nullptr m_stream.
>

> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index 6a1d3964335..b7d3aebda9a 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -420,7 +420,7 @@ class wrapped_file : public ui_file
>    { m_stream->emit_style_escape (style); }
>  
>    int fd () const override
> -  { return m_stream->fd (); }
> +  { return m_stream == nullptr ? -1 : m_stream->fd (); }

Maybe the analysis I linked above is out of date, or I found the wrong
thing, but it seemed to indicate we installed a NULL object into
m_stream at some point because we "restored" something that was never
actually "stored" in the first place.

Tom had a suggestion here:

  https://inbox.sourceware.org/gdb-patches/id:871pt6zwb7.fsf@tromey.com

which I'm not sure was ever followed up on (but I might have missed it,
there are a lot of threads relating to this work).  The idea there is to
avoid restoring something that was never stored.

Another possibility, if we absolutely cannot avoid "restoring" something
that was never stored would be to have the initial state of the thing
that gets restored be some kind of dummy stream whose fd() method return
-1.

My problem with the above is it feels (to me) like throwing a random:
"if (ptr != NULL)" check in because we cannot fix the code that
incorrectly sets 'ptr' to NULL.  But that's just my instinct when
reading the change, maybe the NULL really is unavoidable, it's just the
commit message doesn't convince me of that, it just explains *where* the
NULL is, not *why* the NULL has to be.

Thanks,
Andrew


>  
>    void puts_unfiltered (const char *str) override
>    { m_stream->puts_unfiltered (str); }
> diff --git a/gdb/ui.h b/gdb/ui.h
> index 891660896ef..ef977470302 100644
> --- a/gdb/ui.h
> +++ b/gdb/ui.h
> @@ -163,6 +163,10 @@ struct ui
>      {
>        return current_ui->*F;
>      }
> +    bool operator== (nullptr_t p) const
> +    {
> +      return current_ui->*F == nullptr;
> +    }
>    };
>  
>    /* A ui_file that simply forwards.  */
> diff --git a/gdb/unittests/ui-file-selftests.c b/gdb/unittests/ui-file-selftests.c
> index 69e48735001..b9c1aca3774 100644
> --- a/gdb/unittests/ui-file-selftests.c
> +++ b/gdb/unittests/ui-file-selftests.c
> @@ -48,6 +48,20 @@ run_tests ()
>    scoped_restore save_7 = make_scoped_restore (&sevenbit_strings, true);
>    check_one ("more weird stuff: \xa5", '\\',
>  	     "more weird stuff: \\245");
> +
> +  {
> +    /* There's a bug that has the effect "*redirectable_stderr () == nullptr".
> +       In that case, gdb_stderr is not nullptr, but the underlying pointer is
> +       a nullptr.  Check that "gdb_stderr->fd ()" doesn't dereference the
> +       underlying nullptr.
> +       This allows us to check for a usable gdb_stderr using
> +       "gdb_stderr != nullptr && gdb_stderr->fd () == -1", as we do in
> +       sig_write.  */
> +    scoped_restore restore_stderr
> +      = make_scoped_restore (redirectable_stderr (), nullptr);
> +    SELF_CHECK (gdb_stderr != nullptr);
> +    SELF_CHECK (gdb_stderr->fd () == -1);
> +  }
>  }
>  
>  } /* namespace file*/
>
> base-commit: 8ee110dee1942a20fd206c7107a027e9b04ba56e
> -- 
> 2.51.0