Re: [External] : Re: [PATCH v3] gdb : Signal to pstack/gdb kills the attached process.

[Partha Satapathy <partha.satapathy@oracle.com>]

On 1/10/2024 9:29 PM, Partha Satapathy wrote:
> On 12/5/2023 6:43 PM, Guinevere Larsen wrote:
>> On 17/11/2023 15:48, Partha Satapathy wrote:
>>> On 11/6/2023 7:08 PM, Guinevere Larsen wrote:
>>>> On 02/11/2023 19:27, Partha Satapathy wrote:
>>>>> On 11/2/2023 11:54 PM, Partha Satapathy wrote:
>>>>>> On 10/25/2023 9:24 PM, Guinevere Larsen wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Thanks for working on this issue, and sorry about the delay in 
>>>>>>> getting this reviewed. For future reference, we (at least I) tend 
>>>>>>> to try and go for patches with many pings, so it is better to 
>>>>>>> ping existing patches than re-sending them :)
>>>>>>
>>>>>> I some how missed this mail, hence a delay in reply.
>>>>>> I sorry for this and unfortunately have made the mistake once again.
>>>>>> Hope the extra threads can be deleted and I will keep my 
>>>>>> discussion bound to this thread.
>>>>>>
>>>>>>>
>>>>>>> I'm not very knowledgeable on how GDB does signal handling, so 
>>>>>>> I'm going to review this patch at face value. I hope someone who 
>>>>>>> does know how this part works gets a look at this soon!
>>>>>>>
>>>>>>> On 16/10/2023 11:28, Partha Satapathy wrote:
>>>>>>>> Problem :
>>>>>>>> While gdb attaching a target, If ctrl-c pressed in the midst of 
>>>>>>>> the process attach,  the sigint is passed to the debugged 
>>>>>>>> process. This triggers exit of the debugged.
>>>>>>>>
>>>>>>>> Let’s take the example of pstack,  which dumps the stack of all 
>>>>>>>> threads in a process. In some cases printing of stack can take 
>>>>>>>> significant time and ctrl-c is pressed to abort pstack/gdb 
>>>>>>>> application. This in turn kills the debugged process, which can 
>>>>>>>> be critical for the system. In this case the intention of 
>>>>>>>> “ctrl+c” to kill pstack/gdb, but not the target application.
>>>>>>>>
>>>>>>>> Reproduction:
>>>>>>>>
>>>>>>>> The debugged application generally attached to process by:
>>>>>>>> gdb -p <<pid>>
>>>>>>>> or gdb /proc/<<pid>>/exe pid
>>>>>>>> pstack uses the latter  method to attach the debugged to gdb. If 
>>>>>>>> the application is large or process of reading symbols is slow, 
>>>>>>>> gives a good window to press the ctrl+c during attach. Spawning 
>>>>>>>> "gdb" under "strace -k" makes gdb a lot slower and gives a 
>>>>>>>> larger window to easily press the
>>>>>>>> ctrl+c at the precise period i.e. during the attach of the debugged
>>>>>>>> process. The above strace hack will enhance rate of reproduction 
>>>>>>>> of the issue. Testcase:
>>>>>>>>
>>>>>>>> With GDB 13.1
>>>>>>>> ps aux | grep abrtd
>>>>>>>> root     2195168   /usr/sbin/abrtd -d -s
>>>>>>>>
>>>>>>>> #strace -k -o log gdb -p 2195168
>>>>>>>> Attaching to process 2195168
>>>>>>>> [New LWP 2195177]
>>>>>>>> [New LWP 2195179]
>>>>>>>> ^C[Thread debugging using libthread_db enabled]
>>>>>>>> <<<<   Note the ctrl+c is pressed after attach is initiated and 
>>>>>>>> it’s
>>>>>>>> still reading the symbols from library >>>> Using host 
>>>>>>>> libthread_db library "/lib64/libthread_db.so.1".
>>>>>>>> 0x00007fe3ed6d70d1 in poll () from /lib64/libc.so.6
>>>>>>>> (gdb) q
>>>>>>>> A debugging session is active.
>>>>>>>>           Inferior 1 [process 2195168] will be detached Quit 
>>>>>>>> anyway? (y or n) y Detaching from program: /usr/sbin/abrtd, 
>>>>>>>> process 2195168
>>>>>>>>
>>>>>>>> # ps aux | grep 2195168
>>>>>>>> <<<< Process exited >>>>
>>>>>>>>
>>>>>>>> Description:
>>>>>>>>
>>>>>>>> We are installing a signal handler in gdb that marks the 
>>>>>>>> Ctrl-c/sigint received by gdb. GDB passes this sigint to the 
>>>>>>>> debugged at some definite points during the window of process 
>>>>>>>> attach. The process of attaching debugged involves steps like 
>>>>>>>> PTRACE_ATTACH , reading symbols, getting the stop signal from 
>>>>>>>> the debugged and get ready with GDB prompt. Note:
>>>>>>>> one of the example of this is sigint passing is:
>>>>>>>> "     - installs a SIGINT handler that forwards SIGINT to the 
>>>>>>>> inferior.
>>>>>>>>          Otherwise a Ctrl-C pressed just while waiting for the 
>>>>>>>> initial
>>>>>>>>          stop would end up as a spurious Quit.
>>>>>>>> "
>>>>>>>>
>>>>>>>> There are few other places where sigint is passed to the 
>>>>>>>> debugged during attach of process to gdb. As the debugger and 
>>>>>>>> debugged are not fully attached during this period, the sigint 
>>>>>>>> takes its default action and terminates the process.
>>>>>>>>
>>>>>>>> Solution:
>>>>>>>>
>>>>>>>> While gdb attaches process, the target is not the current 
>>>>>>>> session leader. Hence, until attach is complete and GDB prompt 
>>>>>>>> is availed, the sigint should not be passed to the debugged. A 
>>>>>>>> similar approach is taken for "gdb) run &". In 
>>>>>>>> target_terminal::inferior()
>>>>>>>>     /* A background resume (``run&'') should leave GDB in 
>>>>>>>> control of the
>>>>>>>>        terminal.  */
>>>>>>>>     if (ui->prompt_state != PROMPT_BLOCKED)
>>>>>>>>       return;
>>>>>>>>
>>>>>>>> The passing of signal is skipped if the process ran in 
>>>>>>>> background. With this approach we can skip passing the sigint if 
>>>>>>>> the process is attached to gdb and process attach is not complete.
>>>>>>>> Here is the proposed solution:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Fix :
>>>>>>>>
>>>>>>>> While gdb attaching a target, If ctrl-c/sigint pressed in the 
>>>>>>>> midst of the process attach, the sigint is passed to the 
>>>>>>>> debugged process.
>>>>>>>> This triggers exit of the debugged.
>>>>>>>>
>>>>>>>> This issue is evident while getting the process stack with ./gdb 
>>>>>>>> --quiet -nx  -ex 'set width 0' -ex 'set height 0'
>>>>>>>> -ex 'set pagination no' -ex 'set confirm off'
>>>>>>>> -ex 'thread apply all bt' -ex quit /proc/<PID>/exe <PID> and 
>>>>>>>> press the ctrl+c while attach.
>>>>>>>>
>>>>>>>> The above method is also used in pstack application which is a 
>>>>>>>> wrapper over gdb to print the process stack. A Ctrl+C intended 
>>>>>>>> to kill gdb or pstack, but kills the debugged even if it is 
>>>>>>>> attached and not spawned by gdb.
>>>>>>>
>>>>>>> This is a very good description of the error you've encountered, 
>>>>>>> but given the repetition on this "fix:" part, I'm wondering, what 
>>>>>>> is meant to be the commit message? Is it just these last few 
>>>>>>> lines, or is it the whole thing? If it is just this last bit, I 
>>>>>>> think it would benefit from some more explanation of the 
>>>>>>> solution. If it is the whole message, I think you can reduce a 
>>>>>>> bit the repetition.
>>>>>>>
>>>>>>> Also, at many points you say "debugged process" and "target". In 
>>>>>>> GDB-land we call that the "inferior". Target has a very specific 
>>>>>>> meaning in the context of GDB (roughly the CPU you're running, 
>>>>>>> and some extra bits here and there).
>>>>>>>
>>>>>>> I also have a few comments on the specific changes, that are 
>>>>>>> inlined.
>>>>>>>
>>>>>>>> ---
>>>>>>>>   gdb/inferior.h | 3 +++
>>>>>>>>   gdb/target.c   | 4 ++++
>>>>>>>>   gdb/top.c      | 2 ++
>>>>>>>>   3 files changed, 9 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/gdb/inferior.h b/gdb/inferior.h index 
>>>>>>>> 4d001b0ad50e..b7048d10bbe4 100644
>>>>>>>> --- a/gdb/inferior.h
>>>>>>>> +++ b/gdb/inferior.h
>>>>>>>> @@ -557,6 +557,9 @@ class inferior : public refcounted_object,
>>>>>>>>     /* True if this child process was attached rather than 
>>>>>>>> forked.  */
>>>>>>>>     bool attach_flag = false;
>>>>>>>>
>>>>>>>> +  /* True if target process synced and gdb ui is out of block. 
>>>>>>>> */ bool
>>>>>>>
>>>>>>> This comment is oddly worded. Based on the change to gdb/top.c, I 
>>>>>>> think you could reword it like this:
>>>>>>>
>>>>>>> /* True if inferior has been fully synced and the prompt is no 
>>>>>>> longer blocked.  */
>>>>>>>
>>>>>>>> + sync_flag = false;
>>>>>>> Typo here, the variable's type should be on this line.
>>>>>>>> +
>>>>>>>>     /* If this inferior is a vfork child, then this is the 
>>>>>>>> pointer to
>>>>>>>>        its vfork parent, if GDB is still attached to it.  */
>>>>>>>>     inferior *vfork_parent = NULL;
>>>>>>>> diff --git a/gdb/target.c b/gdb/target.c index 
>>>>>>>> d5bfd7d0849b..f7c115497451 100644
>>>>>>>> --- a/gdb/target.c
>>>>>>>> +++ b/gdb/target.c
>>>>>>>> @@ -3826,6 +3826,10 @@ target_pass_ctrlc (void)
>>>>>>>>                   through the target_stack.  */
>>>>>>>>                scoped_restore_current_inferior restore_inferior;
>>>>>>>>                set_current_inferior (inf);
>>>>>>>> +             if ((current_inferior()->attach_flag) &&
>>>>>>>
>>>>>>> A couple of style issues here: when the indentation would have 8 
>>>>>>> spaces, you should use a tab instead;
>>>>>>>
>>>>>>> There should be a space between the function name and the 
>>>>>>> parameters;
>>>>>>> And when you need to cut a logical expression in half, the 
>>>>>>> operator should be at the start of a new line.
>>>>>>>
>>>>>>>> + !(current_inferior()->sync_flag)) {
>>>>>>> In this case, since it is just one line, there is no need to have 
>>>>>>> the curly braces. However, when they are needed, they should be 
>>>>>>> on the following line, and 2 spaces further in indentation.
>>>>>>>> + return;
>>>>>>>> +             }
>>>>>>>>                current_inferior ()->top_target ()->pass_ctrlc ();
>>>>>>>>                return;
>>>>>>>>              }
>>>>>>>> diff --git a/gdb/top.c b/gdb/top.c
>>>>>>>> index 621aa6883233..26cc6caac0e5 100644
>>>>>>>> --- a/gdb/top.c
>>>>>>>> +++ b/gdb/top.c
>>>>>>>> @@ -542,6 +542,8 @@ wait_sync_command_done (void)
>>>>>>>>     while (gdb_do_one_event () >= 0)
>>>>>>>>       if (ui->prompt_state != PROMPT_BLOCKED)
>>>>>>>>         break;
>>>>>>>> +
>>>>>>>> +  current_inferior()->sync_flag = true;
>>>>>>>
>>>>>>> I'm not very knowledgeable on this part of GDB, so take this with 
>>>>>>> a grain of salt, but I wonder if this is the best place to put this.
>>>>>>>
>>>>>>> Since you only set this flag as false when first creating the 
>>>>>>> inferior structure, I don't see why it should be re-set every 
>>>>>>> time we're waiting for a command to be done. You could set the 
>>>>>>> sync flag to false every command, but that feels like overkill. I 
>>>>>>> feel like there should be some a mechanism in GDB already that 
>>>>>>> knows if we're the session leader or not, and thus handles things 
>>>>>>> correctly, but I don't know what it is.
>>>>>>>
>>>>>>> Another possibility, based on the exact problem you had, is to 
>>>>>>> put this at the end of either symbol expansions, or the reasons 
>>>>>>> they are being expanded in the first place (which I suspect is 
>>>>>>> something like trying to identify the language or name of the 
>>>>>>> main function).
>>>>>>>
>>>>>>
>>>>>> wait_sync_command_done() is not frequently called with command 
>>>>>> execution.
>>>>>> strace -k -o log ./gdb -p <<pid>>
>>>>>> (gdb) ls
>>>>>> Undefined command: "ls".  Try "help".
>>>>>> (gdb) !ls
>>>>>> (gdb) disassemble main
>>>>>>
>>>>>> confirmed the function wait_sync_command_done() is not part of 
>>>>>> this trace. wait_sync_command_done() is called from 
>>>>>> run_inferior_call()
>>>>>> and serve as inferior startup and wait for it to stop.
>>>>>>
>>>>>> /* Subroutine of call_function_by_hand to simplify it.
>>>>>>     Start up the inferior and wait for it to stop.
>>>>>>     Return the exception if there's an error, or an exception with
>>>>>>     reason >= 0 if there's no error.
>>>>>>
>>>>>>     This is done inside a TRY_CATCH so the caller needn't worry about
>>>>>>     thrown errors.  The caller should rethrow if there's an 
>>>>>> error.  */
>>>>>>
>>>>>> static struct gdb_exception
>>>>>> run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
>>>>>>                     struct thread_info *call_thread, CORE_ADDR 
>>>>>> real_pc)
>>>>>> {
>>>>>>
>>>>>>        /* Inferior function calls are always synchronous, even if the
>>>>>>           target supports asynchronous execution.  */
>>>>>>        wait_sync_command_done ();
>>>>>>
>>>>>> So wait_sync_command_done called once per inferior at startup.
>>>>>>
>>>>>>
>>>>>> Hi Guinevere,
>>>>>>
>>>>>> Thanks for the review and sorry for the delay in reply.
>>>>>> Please find comments inline.
>>>>>>
>>>>>> I will send the V2 incorporating rest of the comment.
>>>>>>
>>>>>> Thanks
>>>>>> Partha
>>>>>
>>>> Hi! Thanks for the updated version. It looks much better!
>>>>
>>>> However, I still cant apply the patch. Are you sure you're 
>>>> developing on the master branch of our upstream repository? 
>>>> (https://urldefense.com/v3/__https://sourceware.org/git/binutils-gdb.git__;!!ACWV5N9M2RV99hQ!N_X8-tLG80n66yoOg95U0435CrvbbnDiHbebshHmNxivPGKLL5ZTy2le27VURzCGpKU6zzBqP4Jtu3d5km1jRA$ )
>>>>
>>>> I have been manually changing the source code to test the patch, but 
>>>> it should cleanly apply for other maintainer to have an easier time 
>>>> reviewing things.
>>>>
>>>>>
>>>>> Problem: While gdb is attaching an inferior, if ctrl-c is pressed 
>>>>> in the
>>>>> middle of the process attach,  the sigint is passed to the debugged
>>>>> process. This triggers the exit of the inferior. For example in 
>>>>> pstack,
>>>>> printing a stack can take significant time, and ctrl-c is pressed to
>>>>> abort the pstack/gdb application. This in turn kills the debugged
>>>>> process, which can be critical for the system. In this case, the
>>>>> intention of ctrl+c is to kill pstack/gdb, but not the inferior
>>>>> application.
>>>>> gdb -p <<pid>>
>>>>> or gdb /proc/<<pid>>/exe pid
>>>>> Attaching to process
>>>>> << ctrl+c is pressed during attach
>>>>> (gdb) q
>>>>> <<<< inferior process exited >>>>
>>>>>
>>>>> A Ctrl-C/sigint received by gdb during the attachment of an inferior
>>>>> passed to the debugged at some definite points during the window of
>>>>> process attachment. The process of attaching an inferior is a 
>>>>> multistep
>>>>> process, and it takes time to get ready with the GDB prompt. As the
>>>>> debugger and debugger are not fully attached during this period, the
>>>>> sigint takes its default action to terminate the process.
>>>>>
>>>>> Solution: While GDB attaches processes, the inferior is not the 
>>>>> current
>>>>> session leader. Hence, until attach is complete and the GDB prompt is
>>>>> available, the sigint should not be passed to the inferior.
>>>>> The signal should be skipped if the process runs in the background. 
>>>>> With
>>>>> this approach, we can skip passing the signature if the process is
>>>>> attached to the GDB and the process attach is not complete.
>>>>> ---
>>>>>  gdb/inferior.h | 3 +++
>>>>>  gdb/target.c   | 4 ++++
>>>>>  gdb/top.c      | 2 ++
>>>>>  3 files changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/gdb/inferior.h b/gdb/inferior.h
>>>>> index 4d001b0ad50e..d5d01bd0d09c 100644
>>>>> --- a/gdb/inferior.h
>>>>> +++ b/gdb/inferior.h
>>>>> @@ -557,6 +557,9 @@ class inferior : public refcounted_object,
>>>>>    /* True if this child process was attached rather than forked. */
>>>>>    bool attach_flag = false;
>>>>>
>>>>> +  /* True if inferior has been fully synced and prompt is no 
>>>>> longer blocked.  */
>>>>> +  bool sync_flag = false;
>>>>> +
>>>>>    /* If this inferior is a vfork child, then this is the pointer to
>>>>>       its vfork parent, if GDB is still attached to it.  */
>>>>>    inferior *vfork_parent = NULL;
>>>>> diff --git a/gdb/target.c b/gdb/target.c
>>>>> index d5bfd7d0849b..4eff3130bad7 100644
>>>>> --- a/gdb/target.c
>>>>> +++ b/gdb/target.c
>>>>> @@ -3826,6 +3826,10 @@ struct target_ops *
>>>>>                  through the target_stack.  */
>>>>>               scoped_restore_current_inferior restore_inferior;
>>>>>               set_current_inferior (inf);
>>>>> +             if ((current_inferior ()->attach_flag)
>>>>> +                 && !(current_inferior ()->sync_flag))
>>>>> +                   return;
>>>> also, there's still 8 spaces here. All 8 space-identations should be 
>>>> replaced with tabs.
>>>>> +
>>>>>               current_inferior ()->top_target ()->pass_ctrlc ();
>>>>>               return;
>>>>>             }
>>>>> diff --git a/gdb/top.c b/gdb/top.c
>>>>> index a685dbf5122e..f05fdd161a42 100644
>>>>> --- a/gdb/top.c
>>>>> +++ b/gdb/top.c
>>>>> @@ -542,6 +542,8 @@ struct ui_out **
>>>>>    while (gdb_do_one_event () >= 0)
>>>>>      if (ui->prompt_state != PROMPT_BLOCKED)
>>>>>        break;
>>>>> +
>>>>> +  current_inferior ()->sync_flag = true;
>>>>
>>>> I'm still not 100% convinced this is the best place to put this. 
>>>> Mainly because this function is also called by 
>>>> maybe_wait_sync_command_done; it didn't show up in your testing 
>>>> because when we run gdb from a terminal, the UI is synchronous (so 
>>>> it fails the first part of the IF condition), but this would be 
>>>> exercised in other situations. And maybe_wait_sync_command_done is 
>>>> called after every single command.
>>>>
>>>> I tried adding this to setup_inferior, which looked like the perfect 
>>>> place for it, but it unfortunately didn't work. Since done is better 
>>>> than perfect, I'm not going to block this patch on this, but I'd 
>>>> love to see a more logical place for this code.
>>>>
>>>
>>> Hi Guinevere,
>>>
>>> Can't agree more on setup_inferior is the best place to reset this 
>>> variable. Updated V3 of review accordingly. Added a check_quit_flag 
>>> which will clear quit_flag, if set before setup_inferior.
>>>
>>>
>>> Author: Partha Sarathi Satapathy <partha.satapathy@oracle.com>
>>> Date:   Fri Nov 17 11:42:11 2023 +0000
>>>
>>> gdb : Signal to pstack/gdb kills the attached process.
>>>
>>> Problem: While gdb is attaching an inferior, if ctrl-c is pressed in the
>>> middle of the process attach,  the sigint is passed to the debugged
>>> process. This triggers the exit of the inferior. For example in pstack,
>>> printing a stack can take significant time, and ctrl-c is pressed to
>>> abort the pstack/gdb application. This in turn kills the debugged
>>> process, which can be critical for the system. In this case, the
>>> intention of ctrl+c is to kill pstack/gdb, but not the inferior
>>> application.
>>> gdb -p <<pid>>
>>> or gdb /proc/<<pid>>/exe pid
>>> Attaching to process
>>> << ctrl+c is pressed during attach
>>> (gdb) q
>>> <<<< inferior process exited >>>>
>>>
>>> A Ctrl-C/sigint received by gdb during the attachment of an inferior
>>> passed to the debugged at some definite points during the window of
>>> process attachment. The process of attaching an inferior is a multistep
>>> process, and it takes time to get ready with the GDB prompt. As the
>>> debugger and debugger are not fully attached during this period, the
>>> sigint takes its default action to terminate the process.
>>>
>>> Solution: While GDB attaches processes, the inferior is not the current
>>> session leader. Hence, until attach is complete and the GDB prompt is
>>> available, the sigint should not be passed to the inferior.
>>> The signal should be skipped if the process runs in the background. With
>>> this approach, we can skip passing the signature if the process is
>>> attached to the GDB and the process attach is not complete.
>>
>> Hi!
>>
>> Sorry about the delay on this. I think this is patch looks good to me, 
>> Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
>>
>> I hope some maintainer for this area look at this soon!
>>
> 
> Hi Team,
> 
> Great if we can get a update on further proceedings on this.
> 
> Thanks
> Partha

Hi Guinevere and Team,

Great if we can have further update on this.
One more thing notice the online thread for this issue:
https://sourceware.org/pipermail/gdb-patches/2023-November/204251.html
is missing last couple of communications.

Thanks
Partha