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

[Guinevere Larsen <blarsen@redhat.com>]

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!

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> ---
>  gdb/infcmd.c   | 2 ++
>  gdb/inferior.h | 3 +++
>  gdb/target.c   | 3 +++
>  3 files changed, 8 insertions(+)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index cf8cd527955..0aedbfc06b8 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2556,6 +2556,8 @@ setup_inferior (int from_tty)
>    target_post_attach (inferior_ptid.pid ());
>
>    post_create_inferior (from_tty);
> +  current_inferior ()->sync_flag = true;
> +  check_quit_flag();
>  }
>
>  /* What to do after the first program stops after attaching.  */
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 33eff7a9141..4e517bf9bc4 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -600,6 +600,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 a6ca7fc4f07..b5556eaeb5a 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3811,6 +3811,9 @@ target_pass_ctrlc (void)
>                  through the target_stack.  */
>               scoped_restore_current_inferior restore_inferior;
>               set_current_inferior (inf);
> +             if ((current_inferior ()->attach_flag)
> +               && !(current_inferior ()->sync_flag))
> +                 return;
>               current_inferior ()->top_target ()->pass_ctrlc ();
>               return;
>             }
> -- 
> 2.39.3
>
>
> Thanks
> Partha
>
>