Hi Oliver,

The crash got fixed with the patch you provided before. 

Do you thing the latest patch will be the right solution?

Thanks,
Praveen.

-----Original Message-----
From: Olivier Houchard [mailto:ohouch...@haproxy.com] 
Sent: Friday, April 13, 2018 8:57 AM
To: Willy Tarreau <w...@1wt.eu>
Cc: UPPALAPATI, PRAVEEN <pu5...@att.com>; haproxy@formilux.org
Subject: Re: HAProxy 1.8.X crashing

Hi,

On Thu, Apr 12, 2018 at 11:33:43PM +0200, Willy Tarreau wrote:
> That may be the problem. But if a mux fails to initialize, then we can't
> destroy them either ? Is cs_destroy() the problem here maybe ? If so, I
> suspect that this approach would be more robust and would better match
> the validity tests on conn->mux that are performed at many places in the
> connection code :
> 
>       if (cs->conn->mux) {
>                   cs->conn->mux->detach(cs);
>       } else {
>             conn_stop_tracking(cs->conn);
>             conn_xprt_close(cs->conn);
>             conn_free(cs->conn);
>         }
>         cs_free(cs);
> 
> > >   - 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.
> 
> At least in the current situation we pre-create a connection just to start
> to fill some information (source/destination address), without knowing the
> mux yet. It's an initialization process, it may look beautiful or ugly
> depending on the perspective, but in any case we must ensure that the
> code overall remains reliable in this situation. And to me it looks less
> ugly than picking a possibly wrong mux just to have a valid pointer
> avoiding a segfault.
> 
> But if it turns out that stabilizing the destroy path (or error path) is
> not sufficient and that it breaks somewhere else, we can well adopt this
> solution for the short term and see how to address it for the long term;
> we already know that the outgoing connection code is severely limited to
> the point of currently preventing us from using H2 on the backend, and
> that's exactly why we're currently working on it.

Ok, here is a patch that does exactly what you suggest. I'm not entirely
happy with it, but it'll do the job, as a stopgap. I want this crash
fixed :)

Olivier

Reply via email to