Re: [tabled patch] abstract out TCP-write code

[Pete Zaitcev <zaitcev@xxxxxxxxxx>]

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