Pete Zaitcev wrote:
> On Thu, 23 Sep 2010 00:32:09 -0400
> Jeff Garzik <jeff@xxxxxxxxxx> wrote:
>
>> >>> So, we go a longer route and re-hook the list of completions
>> >>> to a per-server global instead of a client. The patch is straight-
>> >>> forward. The only thing we need to be careful is to make sure
>> >>> that no outstanding completions are left in the queue before
>> >>> freeing a client struct. This is ensured by force-running completions.
>> >
>> >> Looking at this change again, I don't see how this avoids
>> >> use-after-free. If completions exist after state change function leads
>> >> one to cli_evt_dispose() -> cli_free(), then cli_write_run_compl() still
>> >> calls cli_write_free() with the stale 'cli' pointer.
>> >
>> > We run completions before freeing in all cases. My patch was correct.
>>
>> Logically, if completions are run before freeing in all cases, there is
>> no need to make write_compl_q global. That was a red herring, which by
>> side effect avoided the bug with the stale 'cli' pointer.
>
> Side effect or not, if one applies your patch and executes
> "export MALLOC_PERTURB_=43" command before "make check",
> the result is a crash:
>
> Core was generated by `../server/tabled -C ../test/tabled-test.conf -E'.
> Program terminated with signal 11, Segmentation fault.
> #0 atcp_write_free (tmp=0x2b2b2b2b2b2b2afb, done=true) at atcp.c:49
> 49 struct atcp_wr_state *wst = tmp->wst;
> (gdb) where
> #0 atcp_write_free (tmp=0x2b2b2b2b2b2b2afb, done=true) at atcp.c:49
> #1 0x000000000040387e in atcp_write_run_compl (wst=0x15a9be8) at atcp.c:70
> #2 0x000000000040f20a in tcp_cli_event (fd=<value optimized out>, events=2,
> userdata=0x15a9af0) at server.c:1227
> #3 0x00007f9320aec774 in event_base_loop () from /usr/lib64/libevent-1.4.so.2
> #4 0x00000000004107a5 in main (argc=<value optimized out>,
> argv=<value optimized out>) at server.c:2139
> (gdb)
>
> The existing code (with the commit that you criticized) produces no crash.
> Granted MALLOC_PERTURB_ is like SLAB debug option -- is not the normal
> operating environment -- but IMHO it is completely legitimate.
Every developer should have MALLOC_PERTURB_=N (N in 1..255) set in
his/her environment on glibc-based systems. Almost all the time.
Maybe I should push harder to get it added to rawhide's /etc/profile.
I proposed that a few months ago:
use MALLOC_PERTURB_ ... or lose
http://thread.gmane.org/gmane.linux.redhat.fedora.devel/132690
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
