From: Willy Tarreau Date: Mon, 2 May 2022 14:36:47 +0000 (+0200) Subject: MEDIUM: stream: remove the confusing SF_ADDR_SET flag X-Git-Tag: v2.6-dev9~119 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=03bd3952a68439b5b540aef61ca7620169b059f3;p=thirdparty%2Fhaproxy.git MEDIUM: stream: remove the confusing SF_ADDR_SET flag This flag is no longer needed now that it must always match the presence of a destination address on the backend conn_stream. Worse, before previous patch, if it were to be accidently removed while the address is present, it could result in a leak of that address since alloc_dst_address() would first be called to flush it. Its usage has a long history where addresses were stored in an area shared with the connection, but as this is no longer the case, there's no reason for putting this burden onto application-level code that should not focus on setting obscure flags. The only place where that made a small difference is in the dequeuing code in case of queue redistribution, because previously the code would first clear the flag, and only later when trying to deal with the queue, would release the address. It's not even certain whether there would exist a code path going to connect_server() without calling pendconn_dequeue() first (e.g. retries on queue timeout maybe?). Now the pendconn_dequeue() code will rely on SF_ASSIGNED to decide to clear and release the address, since that flag is always set while in a server's queue, and its clearance implies that we don't want to keep the address. At least it remains consistent and there's no more risk of leaking it. --- diff --git a/dev/flags/flags.c b/dev/flags/flags.c index 8052e8d40b..3a2b48ab9d 100644 --- a/dev/flags/flags.c +++ b/dev/flags/flags.c @@ -402,7 +402,6 @@ void show_strm_flags(unsigned int f) SHOW_FLAG(f, SF_MONITOR); SHOW_FLAG(f, SF_FORCE_PRST); SHOW_FLAG(f, SF_BE_ASSIGNED); - SHOW_FLAG(f, SF_ADDR_SET); SHOW_FLAG(f, SF_ASSIGNED); SHOW_FLAG(f, SF_DIRECT); diff --git a/include/haproxy/stream-t.h b/include/haproxy/stream-t.h index 5e3541fafa..4a017f4cd0 100644 --- a/include/haproxy/stream-t.h +++ b/include/haproxy/stream-t.h @@ -37,7 +37,7 @@ /* Various Stream Flags, bits values 0x01 to 0x100 (shift 0) */ #define SF_DIRECT 0x00000001 /* connection made on the server matching the client cookie */ #define SF_ASSIGNED 0x00000002 /* no need to assign a server to this stream */ -#define SF_ADDR_SET 0x00000004 /* this stream's server address has been set */ +/* unused: 0x00000004 */ #define SF_BE_ASSIGNED 0x00000008 /* a backend was assigned. Conns are accounted. */ #define SF_FORCE_PRST 0x00000010 /* force persistence here, even if server is down */ diff --git a/include/haproxy/stream.h b/include/haproxy/stream.h index b77c9930f2..5660857d42 100644 --- a/include/haproxy/stream.h +++ b/include/haproxy/stream.h @@ -339,7 +339,7 @@ static inline void stream_choose_redispatch(struct stream *s) process_srv_queue(objt_server(s->target)); sockaddr_free(&s->csb->dst); - s->flags &= ~(SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET); + s->flags &= ~(SF_DIRECT | SF_ASSIGNED); s->csb->state = CS_ST_REQ; } else { if (objt_server(s->target)) diff --git a/src/backend.c b/src/backend.c index c58445361b..3b0e231c24 100644 --- a/src/backend.c +++ b/src/backend.c @@ -1349,13 +1349,9 @@ static int connect_server(struct stream *s) * it can be NULL for dispatch mode or transparent backend */ srv = objt_server(s->target); - if (!(s->flags & SF_ADDR_SET)) { - err = alloc_dst_address(&s->csb->dst, srv, s); - if (err != SRV_STATUS_OK) - return SF_ERR_INTERNAL; - - s->flags |= SF_ADDR_SET; - } + err = alloc_dst_address(&s->csb->dst, srv, s); + if (err != SRV_STATUS_OK) + return SF_ERR_INTERNAL; err = alloc_bind_address(&bind_addr, srv, s); if (err != SRV_STATUS_OK) @@ -1904,7 +1900,7 @@ int srv_redispatch_connect(struct stream *s) */ if (((s->flags & (SF_DIRECT|SF_FORCE_PRST)) == SF_DIRECT) && (s->be->options & PR_O_REDISP)) { - s->flags &= ~(SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET); + s->flags &= ~(SF_DIRECT | SF_ASSIGNED); sockaddr_free(&s->csb->dst); goto redispatch; } diff --git a/src/cli.c b/src/cli.c index b32b8b1a4b..7f6b58b4ef 100644 --- a/src/cli.c +++ b/src/cli.c @@ -2770,7 +2770,7 @@ int pcli_wait_for_response(struct stream *s, struct channel *rep, int an_bit) s->csb->flags &= CS_FL_ISBACK | CS_FL_DONT_WAKE; /* we're in the context of process_stream */ s->req.flags &= ~(CF_SHUTW|CF_SHUTW_NOW|CF_AUTO_CONNECT|CF_WRITE_ERROR|CF_STREAMER|CF_STREAMER_FAST|CF_NEVER_WAIT|CF_WROTE_DATA); s->res.flags &= ~(CF_SHUTR|CF_SHUTR_NOW|CF_READ_ATTACHED|CF_READ_ERROR|CF_READ_NOEXP|CF_STREAMER|CF_STREAMER_FAST|CF_WRITE_PARTIAL|CF_NEVER_WAIT|CF_WROTE_DATA|CF_READ_NULL); - s->flags &= ~(SF_DIRECT|SF_ASSIGNED|SF_ADDR_SET|SF_BE_ASSIGNED|SF_FORCE_PRST|SF_IGNORE_PRST); + s->flags &= ~(SF_DIRECT|SF_ASSIGNED|SF_BE_ASSIGNED|SF_FORCE_PRST|SF_IGNORE_PRST); s->flags &= ~(SF_CURR_SESS|SF_REDIRECTABLE|SF_SRV_REUSED); s->flags &= ~(SF_ERR_MASK|SF_FINST_MASK|SF_REDISP); s->conn_retries = 0; /* used for logging too */ diff --git a/src/dns.c b/src/dns.c index 29d3cef398..e7e42ff1c4 100644 --- a/src/dns.c +++ b/src/dns.c @@ -917,7 +917,7 @@ static struct appctx *dns_session_create(struct dns_session *ds) s->csb->dst = addr; s->csb->flags |= CS_FL_NOLINGER; s->target = &ds->dss->srv->obj_type; - s->flags = SF_ASSIGNED|SF_ADDR_SET; + s->flags = SF_ASSIGNED; s->do_log = NULL; s->uniq_id = 0; diff --git a/src/hlua.c b/src/hlua.c index 3dd2667102..06fa163dc0 100644 --- a/src/hlua.c +++ b/src/hlua.c @@ -2781,7 +2781,6 @@ __LJMP static int hlua_socket_connect(struct lua_State *L) xref_unlock(&socket->xref, peer); WILL_LJMP(luaL_error(L, "connect: internal error")); } - s->flags |= SF_ADDR_SET; /* Get hlua struct, or NULL if we execute from main lua state */ hlua = hlua_gethlua(L); diff --git a/src/http_client.c b/src/http_client.c index e12d556e0f..0d34ca8c6d 100644 --- a/src/http_client.c +++ b/src/http_client.c @@ -536,7 +536,7 @@ struct appctx *httpclient_start(struct httpclient *hc) s->csb->dst = addr; s->csb->flags |= CS_FL_NOLINGER; - s->flags |= SF_ASSIGNED|SF_ADDR_SET; + s->flags |= SF_ASSIGNED; s->res.flags |= CF_READ_DONTWAIT; /* applet is waiting for data */ diff --git a/src/peers.c b/src/peers.c index 3c396958a3..522f2dbd82 100644 --- a/src/peers.c +++ b/src/peers.c @@ -3209,7 +3209,7 @@ static struct appctx *peer_session_create(struct peers *peers, struct peer *peer /* initiate an outgoing connection */ s->csb->dst = addr; s->csb->flags |= CS_FL_NOLINGER; - s->flags = SF_ASSIGNED|SF_ADDR_SET; + s->flags = SF_ASSIGNED; s->target = peer_session_target(peer, s); s->do_log = NULL; diff --git a/src/queue.c b/src/queue.c index c2dd981d6b..94f9da8586 100644 --- a/src/queue.c +++ b/src/queue.c @@ -495,7 +495,7 @@ int pendconn_redistribute(struct server *s) /* it's left to the dispatcher to choose a server */ __pendconn_unlink_srv(p); - p->strm_flags &= ~(SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET); + p->strm_flags &= ~(SF_DIRECT | SF_ASSIGNED); task_instant_wakeup(p->strm->task, TASK_WOKEN_RES); xferred++; @@ -595,11 +595,11 @@ int pendconn_dequeue(struct stream *strm) /* the pendconn is not queued anymore and will not be so we're safe * to proceed. */ - strm->flags &= ~(SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET); - strm->flags |= p->strm_flags & (SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET); + strm->flags &= ~(SF_DIRECT | SF_ASSIGNED); + strm->flags |= p->strm_flags & (SF_DIRECT | SF_ASSIGNED); /* the entry might have been redistributed to another server */ - if (!(strm->flags & SF_ADDR_SET)) + if (!(strm->flags & SF_ASSIGNED)) sockaddr_free(&strm->csb->dst); if (p->target) { diff --git a/src/sink.c b/src/sink.c index b89ebb925c..1ef2bacbf7 100644 --- a/src/sink.c +++ b/src/sink.c @@ -660,7 +660,7 @@ static struct appctx *sink_forward_session_create(struct sink *sink, struct sink s->csb->flags |= CS_FL_NOLINGER; s->target = &sft->srv->obj_type; - s->flags = SF_ASSIGNED|SF_ADDR_SET; + s->flags = SF_ASSIGNED; s->do_log = NULL; s->uniq_id = 0;