[SSSD] [PATCH] proxy: Do not crash if proxy_child returns error

Pavel Březina pbrezina at redhat.com
Tue May 26 09:04:07 UTC 2015


On 05/26/2015 10:57 AM, Lukas Slebodnik wrote:
> On (26/05/15 10:44), Pavel Březina wrote:
>> On 05/25/2015 02:58 PM, Lukas Slebodnik wrote:
>>> On (22/05/15 11:02), Pavel Březina wrote:
>>>> On 05/20/2015 02:20 PM, Lukas Slebodnik wrote:
>>>>> ehlo,
>>>>>
>>>>> attached patch fixes a crash if return code of proxy_child is not 0.
>>>>> You can reproduce it with sssd in non-root mode or
>>>>> you can replace proxy_child with bash script.
>>>>>
>>>>> I'm not sure about the name of tevent_immediate callback
>>>>> so feel free to propose better name.
>>>>>
>>>>> LS
>>>>
>>>> Hi,
>>>> the whole proxy auth code does not really follow tevent style but I think
>>>> your name is good enough.
>>>> However, I don't see why it should crash, can you
>>>> explain it in more detail please?
>>>
>>> a) proxy_child_init_send is called
>>>     -- request with state "struct pc_init_ctx" is created
>>>     -- process is forked
>>>     -- parent register signal SIGCHLD with clallback pc_init_sig_handler
>>> b) child return non-zero
>>>     -- parent recieve pc_init_sig_handler
>>> c) pc_init_sig_handler is executed
>>>     -- unused argument "__siginfo" contains NON null data
>>>     -- tevent_req_error(req, EIO) is called
>>>     -- proxy_child_init_done callback is called
>>>     -- request is released (including "struct tevent_signal" and all related data)
>>>     -- unused argument "__siginfo" is NULL now
>>> d) function pc_init_sig_handler finished
>>>     -- we are back in tevent code from singnal handler
>>>     -- tevent wants to do some clean-up and dereference NULL in memset
>>>
>>> It is not clear from back trace but the 1st argument of memset is NULL.
>>>
>>> (gdb) bt
>>> #0  0x00007fd7ba400505 in memset (__len=<optimized out>, __ch=<optimized out>, __dest=<optimized out>) at /usr/include/bits/string3.h:84
>>> #1  tevent_common_check_signal (ev=0x7fd7bfddf670) at ../tevent_signal.c:459
>>> #2  0x00007fd7ba40228c in epoll_event_loop (tvalp=0x7fff85536430, epoll_ev=0x7fd7bfddf8b0) at ../tevent_epoll.c:647
>>> #3  epoll_event_loop_once (ev=<optimized out>, location=<optimized out>) at ../tevent_epoll.c:926
>>> #4  0x00007fd7ba4007d7 in std_event_loop_once (ev=0x7fd7bfddf670, location=0x7fd7bdb417c3 "src/util/server.c:668") at ../tevent_standard.c:114
>>> #5  0x00007fd7ba3fcfbd in _tevent_loop_once (ev=ev at entry=0x7fd7bfddf670, location=location at entry=0x7fd7bdb417c3 "src/util/server.c:668") at ../tevent.c:530
>>> #6  0x00007fd7ba3fd15b in tevent_common_loop_wait (ev=0x7fd7bfddf670, location=0x7fd7bdb417c3 "src/util/server.c:668") at ../tevent.c:634
>>> #7  0x00007fd7ba400777 in std_event_loop_wait (ev=0x7fd7bfddf670, location=0x7fd7bdb417c3 "src/util/server.c:668") at ../tevent_standard.c:140
>>> #8  0x00007fd7bdb29343 in server_loop (main_ctx=0x7fd7bfde0ac0) at src/util/server.c:668
>>> #9  0x00007fd7be39ca42 in main (argc=8, argv=<optimized out>) at src/providers/data_provider_be.c:2909
>>>
>>>
>>> BTW it's easily reproducible with proxy provider. I can give you an access
>>> to such machine.
>>>
>>> I hope it is clear now.
>>>
>>> LS
>>
>> Hi,
>> I'm sorry, but I don't think this is the problem. I'm not saying the solution
>> may not be right, but the cause is different.
>>
>> 1) siginfo is not attached to tevent_signal context, so freeing tevent_signal
>> does not do anything with siginfo
>>
>>> static struct tevent_sig_state {
>>> 	struct tevent_common_signal_list *sig_handlers[TEVENT_NUM_SIGNALS+1];
>>> 	struct sigaction *oldact[TEVENT_NUM_SIGNALS+1];
>>> 	struct tevent_sigcounter signal_count[TEVENT_NUM_SIGNALS+1];
>>> 	struct tevent_sigcounter got_signal;
>>> #ifdef SA_SIGINFO
>>> 	/* with SA_SIGINFO we get quite a lot of info per signal */
>>> 	siginfo_t *sig_info[TEVENT_NUM_SIGNALS+1];
>>> 	struct tevent_sigcounter sig_blocked[TEVENT_NUM_SIGNALS+1];
>>> #endif
>>> } *sig_state;
>>
>> 2) it is a ring buffer, that is the reason why it is cleared in tevent after
>> sighandler is finished
>>
> Yes, it is pointer to ringbuffer.
> But argument "__siginfo" was valid pointer to ringbuffer before calling
> tevent_req_error and after it was NULL. That's the reason why it crashed.
> Dereference of null pointer (in memset)

Yes. But the reason why it is NULL should not be that we freed 
tevent_signal from the handler (unless it is a bug in tevent).

>> 5) I'm not sure why it crashes, however we can stop using flag SA_SIGINFO
>> since it is unused anyway, it should do the trick
>>
> That might be a solution too.
>
>> 6) freeing tevent_signal causes to run destructor of pc_init_ctx which sends
>> kill(SIGKILL). Maybe it triggers some tevent bug?
> Do you want to access to a machine to confirm wheter it's tevent bug or no?
> I do not want to dig in tevent :-)

Sure.




More information about the sssd-devel mailing list