Hi Willy,

On Thu, Apr 12, 2018 at 08:53:51AM +0200, Willy Tarreau wrote:
> Hi Olivier,
> 
> On Wed, Apr 11, 2018 at 05:29:15PM +0200, Olivier Houchard wrote:
> > From 7c9f06727cf60acf873353ac71283ff9c562aeee Mon Sep 17 00:00:00 2001
> > From: Olivier Houchard <ohouch...@haproxy.com>
> > Date: Wed, 11 Apr 2018 17:23:17 +0200
> > Subject: [PATCH] BUG/MINOR: connection: Setup a mux when in proxy mode.
> > 
> > We were allocating a new connection when in proxy mode, but did not provide
> > it a mux, thus crashing later.
> > 
> > This should be backported to 1.8.
> > ---
> >  src/proto_http.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/proto_http.c b/src/proto_http.c
> > index 80e001d69..817692c48 100644
> > --- a/src/proto_http.c
> > +++ b/src/proto_http.c
> > @@ -62,6 +62,7 @@
> >  #include <proto/h1.h>
> >  #include <proto/log.h>
> >  #include <proto/hdr_idx.h>
> > +#include <proto/mux_pt.h>
> >  #include <proto/pattern.h>
> >  #include <proto/proto_tcp.h>
> >  #include <proto/proto_http.h>
> > @@ -3718,6 +3719,8 @@ int http_process_request(struct stream *s, struct 
> > channel *req, int an_bit)
> >  
> >                     return 0;
> >             }
> > +           /* XXX: We probably need a better mux */
> > +           conn_install_mux(conn, &mux_pt_ops, objt_cs(s->si[1].end));
> >  
> >             path = http_get_path(txn);
> >             url2sa(req->buf->p + msg->sl.rq.u,
> 
> While I can understand how missing this can have caused trouble, I'm not
> sure it's the best solution, and I'm a bit worried about having various
> code places choose a mux and install it. I suspect we're "simply" missing
> a test in the backend code to cover the case where the connection already
> exists but a mux was not yet installed (a situation that didn't exist
> prior to muxes which is why we didn't notice this). I think that this
> happens in connect_server() around line 1177 (return SF_ERR_INTERNAL)
> when conn_xprt_ready() indicates the transport was not yet initialized.
> It's likely that the error unrolling fails to consider the case where
> the mux wasn't initialized, leading to the crash Praveen experienced.
> 

I'm not sure about this. My point was to try to install the mux when we can
choose one, the day we have more relevant muxes, if it ever happens.
This certainly fixes Praveen's crash.
I don't even know how we could reach line 1177.

> If this is right, it would mean two fixes :
>   - one in the error unrolling path to ensure we don't destroy a non
>     allocated mux ;

And what do we do then ? Without the mux we can't destroy the cs nor the
connection.

>   - one above the return SF_ERR_INTERNAL above in connect_server() to
>     handle the case where the connection already exists but not the mux.

I'm not sure how this can happen, and what we should do if that happens.

Regards,

Olivier

Reply via email to