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

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

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