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@entry=0x7fd7bfddf670, 
location=location@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

3) freeing tevent_signal inside the handler is a valid operation, you can see a pretty clever check whether the structure was freed or not in the tevent code

                        /*
                         * We have to be careful to not touch "se"
                         * after it was deleted in its handler. Thus
                         * we allocate a child whose destructor will
                         * tell by nulling out itself that its parent
                         * is gone.
                         */
                        exists = talloc(se, struct tevent_se_exists);
                        if (exists == NULL) {
                                continue;
                        }
                        exists->myself = &exists;
                        talloc_set_destructor(
                                exists, tevent_se_exists_destructor);

4) so it is definitely not supposed to crash because you free tevent_signal inside the handler

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

6) freeing tevent_signal causes to run destructor of pc_init_ctx which sends kill(SIGKILL). Maybe it triggers some tevent bug?


_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Reply via email to