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

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

On 1/24/2024 8:49 PM, Partha Satapathy wrote:
> 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

Hi Team,

Can you please help with further proceedings on this.

Thanks
Partha