Hi Willy, <blush> Yes, I changed my variable names after testing to clean up and failed. Is my obvious corrected patch the correct fix? Or should we clamp down on the use of global chunks being passed downstream?
--Dave On Wed, Jul 16, 2014 at 4:16 PM, Willy Tarreau <[email protected]> wrote: > Hi Dave, > > On Wed, Jul 16, 2014 at 02:16:52PM -0400, Dave McCowan wrote: > > Hi Willy, Emeric-- > > A commit on 6/24 changed the way ssl_sock_get_remote_common_name() > > works. > > I agree with this refactoring, unfortunately both > make_proxy_line_v2() > > and the caller of make_proxy_line_v2() are using the global trash chunk > as > > a workspace resulting in a memory overwrite. > > Argh! yes you're right, make_proxy_line() is called with trash which is > not that fun. trash is supposed to be overwritable by any function call > which is exactly what happens here since the function became deeper. > > > I've attached a patch to fix this. > > > > Commit comment: Use temporary trash chunk, instead of global trash chunk > in > > make_proxy_line_v2() to avoid memory corruption. > > Are you sure there is not a bug here : > > > if (srv->pp_opts & SRV_PP_V2_SSL_CN) { > > + cn_trash = get_trash_chunk(); > > if > (ssl_sock_get_remote_common_name(remote, &trash) > 0) { > > - tlv_len = > make_tlv(&buf[ret+ssl_tlv_len], (buf_len - ret - ssl_tlv_len), > PP2_TYPE_SSL_CN, trash.len, trash.str); > > + tlv_len = > make_tlv(&buf[ret+ssl_tlv_len], (buf_len - ret - ssl_tlv_len), > PP2_TYPE_SSL_CN, cn_trash->len, cn_trash->str); > > > I'm seeing ssl_sock_get_remote_common_name() write into trash > and make_tlv() retrieve from cn_trash which is still empty, so > I don't know how this could have worked in your tests, so maybe > you checked then cleaned up a bit too much before sending the > patch ? > > Thanks, > Willy > >
