Re: [PATCH v2] Fix logic in exec_file_locate_attach

Luis Machado
2016-02-22T22:50:05Z

On 02/22/2016 07:00 PM, Luis Machado wrote:
> On 02/22/2016 10:51 AM, Gary Benson wrote:
>> Luis Machado wrote:
>>> On 02/22/2016 07:40 AM, Gary Benson wrote:
>>>> Luis Machado wrote:
>>>>> On 02/19/2016 09:21 AM, Gary Benson wrote:
>>>>>> This is an updated version of the patch I posted yesterday.
>>>>>> It fails silently rather than throwing if the executable is
>>>>>> not in the sysroot, which both fixes the sysroot-escape issue
>>>>>> and results in a better GDB session for the user.
>>>>>>
>>>>>> Built and regtested on RHEL 6.6 x86_64.
>>>>>>
>>>>>> Luis, I think this patch will fix your connection drop without
>>>>>> any further changes. Could you test it please?
>>>>>
>>>>> Unfortunately it doesn't completely solve the problem i saw, as
>>>>> exec_file_find will still potentially throw errors and will
>>>>> disrupt the connection attempt or stop execution of a custom
>>>>> sequence of commands (as Pedro noted) when "attach" is part of
>>>>> the sequence.
>>>>>
>>>>> define foo
>>>>> attach <pid>
>>>>>>>>> execution stops here if an error is thrown
>>>>> info threads
>>>>> info registers
>>>>> end
>>>>>
>>>>> It still looks like a TRY/CATCH block is needed around at least
>>>>> exec_file_find.
>>>>
>>>> What is throwing in exec_file_find? I'm just seeing lots of calls
>>>> to gdb_open_cloexec and openp, and I don't think either of those
>>>> should throw except for assertion failures or running out of
>>>> memory.
>>>
>>> Not sure why i had exec_file_find in my mind. I meant to say
>>> exec_file_attach still throws errors, when openp fails and
>>> scratch_chan < 0. Sorry.
>>
>> You shouldn't get that now, the "if (full_exec_path == NULL) return"
>> should have caught it. Are you still seeing thrown errors with your
>> setup?
>>
>
> Yes. With your patch applied, i still see a case where we error out.
> Suppose we have a test binary gdb/test, then:
>
> - chmod -r gdb/test
> - Fire up gdbserver with a test binary: ./gdb/gdbserver/gdbserver :2345
> gdb/test
> - Fire up GDB: ./gdb/gdb -ex "set sysroot" -ex "tar rem :2345"
>
> You will see something similar to the following:
>
> Sending packet: $qXfer:exec-file:read:3486:0,fff#5f...Packet received:
> l/proc/13446/exe
> /proc/13446/exe: Permission denied.
> (gdb) i r
> The program has no registers now.
> (gdb)
>
> This was the testcase suggested by Pedro and it proved to be a good one.
>
>>> There is a symbol_file_add_main call right after calling
>>> exec_file_attach in exec_file_locate_attach, but i didn't see any
>>> errors being thrown from that one.
>>
>> You could probably race it (e.g. by deleting the file between the
>> calls) but generally symbol_file_add_main won't fail because
>> exec_file_attach would have failed if the file was missing or
>> inaccessible.
>
> My idea was to guard both exec_file_attach and symbol_file_add_main. We
> can't have anything in that function throwing an error that won't be
> caught, otherwise the above connection attempt will fail.
>
> Luis
>

For the record, you patch does fix the case of native GDB trying to
attach to a process without pre-loading a symbol file. We get a
multi-frame backtrace as expected.

It is the gdb/gdbserver case that still seems to be broken.