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

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

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