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.
-- Pete
--
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
