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

[Tom de Vries <tdevries@suse.de>]

On 4/21/26 6:13 PM, Andrew Burgess wrote:
> 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.
> 

Hi Andrew,

thanks for the review.

> It would be nice if the commit message explained *why* there's a nullptr
> there.
Ack, I've submitted a v3 ( 
https://sourceware.org/pipermail/gdb-patches/2026-April/226736.html ) 
explaining that briefly, by referring to the commit that fixed that problem.

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

That commit indeed fixes the problem that current_ui.m_ui_stderr is nullptr.

What this patch fixes is that sigwrite is not robust enough to handle 
the situation that current_ui.m_ui_stderr is nullptr, which shouldn't 
but could happen.  We want sigwrite to be as robust as possible because 
it's something that gets called when things go wrong.

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

Right, I have not followed up on that.  I've filed a PR about it now ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=34100 ).

It's a valid suggestion, unfortunately it didn't have any relevance for 
the actual problem at hand, which was a segfault while trying to report 
a segfault.

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

This is not about not being able to fix code, this is about robustly 
handling the case that something that shouldn't happen does happen.

Agreed, the situation that current_ui.m_ui_stderr is nullptr is 
interesting, and normally we'd like to report it, but the exceptional 
situation is that we're trying to report something else, but can't.

Of course there is an easy solution:
...
void
sig_write (const char *msg)
{
-  if (gdb_stderr == nullptr || gdb_stderr->fd () == -1)
     std::ignore = ::write (2, msg, strlen (msg));
-  else
-    gdb_stderr->write_async_safe (msg, strlen (msg));
}
...
but that is probably breaking some ui invariant.

I hope this clarifies my intention with this patch.

Thanks,
- Tom

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