Re: [PATCH 2/2] [gdb] Handle nullptr stream in gdb_flush

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

On 4/28/25 17:21, Simon Marchi wrote:
> On 4/25/25 1:18 PM, Tom de Vries wrote:
>> Using the trigger patch described in the previous commit, I get:
>> ...
>> $ gdb
>> (gdb) <q>error detected on stdin
>>
>> Fatal signal: Segmentation fault
>> ----- Backtrace -----
>> 0x64c7b3 gdb_internal_backtrace_1
>> 	/data/vries/gdb/src/gdb/bt-utils.c:127
>> 0x64c937 _Z22gdb_internal_backtracev
>> 	/data/vries/gdb/src/gdb/bt-utils.c:196
>> 0x94db83 handle_fatal_signal
>> 	/data/vries/gdb/src/gdb/event-top.c:1021
>> 0x94dd48 handle_sigsegv
>> 	/data/vries/gdb/src/gdb/event-top.c:1098
>> 0x7f372be578ff ???
>> 0x10b7c0a _Z9gdb_flushP7ui_file
>> 	/data/vries/gdb/src/gdb/utils.c:1527
>> 0xd4b938 gdbpy_flush
>> 	/data/vries/gdb/src/gdb/python/python.c:1624
>> 0x7f372d73b276 _PyCFunction_FastCallDict
>> 	Objects/methodobject.c:231
>> 0x7f372d73b276 _PyCFunction_FastCallKeywords
>> 	Objects/methodobject.c:294
>> 0x7f372d794a09 call_function
>> 	Python/ceval.c:4851
>> 0x7f372d78e838 _PyEval_EvalFrameDefault
>> 	Python/ceval.c:3351
>> 0x7f372d796e6e PyEval_EvalFrameEx
>> 	Python/ceval.c:754
>> 0x7f372d796e6e _PyFunction_FastCall
>> 	Python/ceval.c:4933
>> 0x7f372d796e6e _PyFunction_FastCallDict
>> 	Python/ceval.c:5035
>> 0x7f372d6fefc8 _PyObject_FastCallDict
>> 	Objects/abstract.c:2310
>> 0x7f372d6fefc8 _PyObject_Call_Prepend
>> 	Objects/abstract.c:2373
>> 0x7f372d6fe162 _PyObject_FastCallDict
>> 	Objects/abstract.c:2331
>> 0x7f372d700705 callmethod
>> 	Objects/abstract.c:2583
>> 0x7f372d700705 _PyObject_CallMethodId
>> 	Objects/abstract.c:2640
>> 0x7f372d812a41 flush_std_files
>> 	Python/pylifecycle.c:699
>> 0x7f372d81281d Py_FinalizeEx
>> 	Python/pylifecycle.c:768
>> 0xd4d49b finalize_python
>> 	/data/vries/gdb/src/gdb/python/python.c:2308
>> 0x9587eb _Z17ext_lang_shutdownv
>> 	/data/vries/gdb/src/gdb/extension.c:330
>> 0xfd98df _Z10quit_forcePii
>> 	/data/vries/gdb/src/gdb/top.c:1817
>> 0x6b3080 _Z12quit_commandPKci
>> 	/data/vries/gdb/src/gdb/cli/cli-cmds.c:483
>> 0x1056577 stdin_event_handler
>> 	/data/vries/gdb/src/gdb/ui.c:131
>> 0x1986970 handle_file_event
>> 	/data/vries/gdb/src/gdbsupport/event-loop.cc:551
>> 0x1986f4b gdb_wait_for_event
>> 	/data/vries/gdb/src/gdbsupport/event-loop.cc:672
>> 0x1985e0c _Z16gdb_do_one_eventi
>> 	/data/vries/gdb/src/gdbsupport/event-loop.cc:263
>> 0xb66f2e start_event_loop
>> 	/data/vries/gdb/src/gdb/main.c:402
>> 0xb670ba captured_command_loop
>> 	/data/vries/gdb/src/gdb/main.c:466
>> 0xb68b9b captured_main
>> 	/data/vries/gdb/src/gdb/main.c:1344
>> 0xb68c44 _Z8gdb_mainP18captured_main_args
>> 	/data/vries/gdb/src/gdb/main.c:1363
>> 0x41a3b1 main
>> 	/data/vries/gdb/src/gdb/gdb.c:38
>> ---------------------
>> A fatal error internal to GDB has been detected, further
>> debugging is not possible.  GDB will now terminate.
>>
>> This is a bug, please report it.  For instructions, see:
>> <https://www.gnu.org/software/gdb/bugs/>.
>>
>> Segmentation fault (core dumped)
>> $ q
>> ...
>>
>> Fix this in gdb_flush by checking for nullptr stream parameter, such that we
>> get instead:
>> ...
>> $ gdb
>> (gdb) <q>error detected on stdin
>> $ q
>> ...
>>
>> Tested on x86_64-linux.
>> ---
>>   gdb/utils.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/utils.c b/gdb/utils.c
>> index ce3c26ef140..90063e4a96d 100644
>> --- a/gdb/utils.c
>> +++ b/gdb/utils.c
>> @@ -1524,7 +1524,8 @@ pager_file::flush ()
>>   void
>>   gdb_flush (struct ui_file *stream)
>>   {
>> -  stream->flush ();
>> +  if (stream != nullptr)
>> +    stream->flush ();
> 

Hi Simon,

thanks for the review.

> Can we add the nullptr check only where it's needed?
> 

AFAIU, I've added it where it's needed.

Can you tell me where you think it's needed?  I cannot tell from your 
comments.

Thanks,
- Tom

> I'm thinking that we should move towards removing gdb_flush (replacing
> the call sites to call ui_file::flush directly).  And when we do so, we
> wouldn't want to add nullptr checks everywhere that gdb_flush is
> currently called.
> 
> Simon