From: Willy Tarreau Date: Mon, 6 Dec 2021 07:01:02 +0000 (+0000) Subject: BUILD: tree-wide: avoid warnings caused by redundant checks of obj_types X-Git-Tag: v2.6-dev1~325 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=88bc800eae8d0a2118a273afc52ecdc529c9f523;p=thirdparty%2Fhaproxy.git BUILD: tree-wide: avoid warnings caused by redundant checks of obj_types At many places we use construct such as: if (objt_server(blah)) do_something(objt_server(blah)); At -O2 the compiler manages to simplify the operation and see that the second one returns the same result as the first one. But at -O1 that's not always the case, and the compiler is able to emit a second expression and sees the potential null that results from it, and may warn about a potential null deref (e.g. with gcc-6.5). There are two solutions to this: - either the result of the first test has to be passed to a local variable - or the second reference ought to be unchecked using the __objt_* variant. This patch fixes all occurrences at once by taking the second approach (the least intrusive). For constructs like: objt_server(blah) ? objt_server(blah)->name : "no name" a macro could be useful. It would for example take the object type (server), the field name (name) and the default value. But there are probably not enough occurrences across the whole code for this to really matter. This should be backported wherever it applies. --- diff --git a/src/backend.c b/src/backend.c index 2d2c0d6e09..fb6313181f 100644 --- a/src/backend.c +++ b/src/backend.c @@ -2230,7 +2230,7 @@ void back_handle_st_cer(struct stream *s) /* we probably have to release last stream from the server */ if (objt_server(s->target)) { - health_adjust(objt_server(s->target), HANA_STATUS_L4_ERR); + health_adjust(__objt_server(s->target), HANA_STATUS_L4_ERR); if (s->flags & SF_CURR_SESS) { s->flags &= ~SF_CURR_SESS; diff --git a/src/http_ana.c b/src/http_ana.c index 036ae109d1..9c3e68a1be 100644 --- a/src/http_ana.c +++ b/src/http_ana.c @@ -1328,7 +1328,7 @@ int http_wait_for_response(struct stream *s, struct channel *rep, int an_bit) struct connection *conn = NULL; if (objt_cs(s->si[1].end)) - conn = objt_cs(s->si[1].end)->conn; + conn = __objt_cs(s->si[1].end)->conn; /* Perform a L7 retry because server refuses the early data. */ if ((si_b->flags & SI_FL_L7_RETRY) && @@ -1889,13 +1889,13 @@ int http_process_res_common(struct stream *s, struct channel *rep, int an_bit, s * requests and this one isn't. Note that servers which don't have cookies * (eg: some backup servers) will return a full cookie removal request. */ - if (!objt_server(s->target)->cookie) { + if (!__objt_server(s->target)->cookie) { chunk_printf(&trash, "%s=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/", s->be->cookie_name); } else { - chunk_printf(&trash, "%s=%s", s->be->cookie_name, objt_server(s->target)->cookie); + chunk_printf(&trash, "%s=%s", s->be->cookie_name, __objt_server(s->target)->cookie); if (s->be->cookie_maxidle || s->be->cookie_maxlife) { /* emit last_date, which is mandatory */ @@ -1968,10 +1968,10 @@ int http_process_res_common(struct stream *s, struct channel *rep, int an_bit, s * the 'checkcache' option, and send an alert. */ ha_alert("Blocking cacheable cookie in response from instance %s, server %s.\n", - s->be->id, objt_server(s->target) ? objt_server(s->target)->id : ""); + s->be->id, objt_server(s->target) ? __objt_server(s->target)->id : ""); send_log(s->be, LOG_ALERT, "Blocking cacheable cookie in response from instance %s, server %s.\n", - s->be->id, objt_server(s->target) ? objt_server(s->target)->id : ""); + s->be->id, objt_server(s->target) ? __objt_server(s->target)->id : ""); goto deny; } @@ -5006,8 +5006,8 @@ static void http_debug_stline(const char *dir, struct stream *s, const struct ht chunk_printf(&trash, "%08x:%s.%s[%04x:%04x]: ", s->uniq_id, s->be->id, dir, - objt_conn(sess->origin) ? (unsigned short)objt_conn(sess->origin)->handle.fd : -1, - objt_cs(s->si[1].end) ? (unsigned short)objt_cs(s->si[1].end)->conn->handle.fd : -1); + objt_conn(sess->origin) ? (unsigned short)__objt_conn(sess->origin)->handle.fd : -1, + objt_cs(s->si[1].end) ? (unsigned short)__objt_cs(s->si[1].end)->conn->handle.fd : -1); max = HTX_SL_P1_LEN(sl); UBOUND(max, trash.size - trash.data - 3); @@ -5037,8 +5037,8 @@ static void http_debug_hdr(const char *dir, struct stream *s, const struct ist n chunk_printf(&trash, "%08x:%s.%s[%04x:%04x]: ", s->uniq_id, s->be->id, dir, - objt_conn(sess->origin) ? (unsigned short)objt_conn(sess->origin)->handle.fd : -1, - objt_cs(s->si[1].end) ? (unsigned short)objt_cs(s->si[1].end)->conn->handle.fd : -1); + objt_conn(sess->origin) ? (unsigned short)__objt_conn(sess->origin)->handle.fd : -1, + objt_cs(s->si[1].end) ? (unsigned short)__objt_cs(s->si[1].end)->conn->handle.fd : -1); max = n.len; UBOUND(max, trash.size - trash.data - 3); diff --git a/src/proto_tcp.c b/src/proto_tcp.c index 83d93fd64a..4aaac9d82f 100644 --- a/src/proto_tcp.c +++ b/src/proto_tcp.c @@ -271,11 +271,11 @@ int tcp_connect_server(struct connection *conn, int flags) switch (obj_type(conn->target)) { case OBJ_TYPE_PROXY: - be = objt_proxy(conn->target); + be = __objt_proxy(conn->target); srv = NULL; break; case OBJ_TYPE_SERVER: - srv = objt_server(conn->target); + srv = __objt_server(conn->target); be = srv->proxy; /* Make sure we check that we have data before activating * TFO, or we could trigger a kernel issue whereby after diff --git a/src/proto_uxst.c b/src/proto_uxst.c index b615a8eae5..663369db43 100644 --- a/src/proto_uxst.c +++ b/src/proto_uxst.c @@ -214,11 +214,11 @@ static int uxst_connect_server(struct connection *conn, int flags) switch (obj_type(conn->target)) { case OBJ_TYPE_PROXY: - be = objt_proxy(conn->target); + be = __objt_proxy(conn->target); srv = NULL; break; case OBJ_TYPE_SERVER: - srv = objt_server(conn->target); + srv = __objt_server(conn->target); be = srv->proxy; break; default: diff --git a/src/stream.c b/src/stream.c index 04d2845651..8f1da76257 100644 --- a/src/stream.c +++ b/src/stream.c @@ -624,8 +624,8 @@ static void stream_free(struct stream *s) s->flags &= ~SF_CURR_SESS; _HA_ATOMIC_DEC(&__objt_server(s->target)->cur_sess); } - if (may_dequeue_tasks(objt_server(s->target), s->be)) - process_srv_queue(objt_server(s->target)); + if (may_dequeue_tasks(__objt_server(s->target), s->be)) + process_srv_queue(__objt_server(s->target)); } if (unlikely(s->srv_conn)) { @@ -826,7 +826,7 @@ void stream_process_counters(struct stream *s) _HA_ATOMIC_ADD(&s->be->be_counters.bytes_in, bytes); if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.bytes_in, bytes); + _HA_ATOMIC_ADD(&__objt_server(s->target)->counters.bytes_in, bytes); if (sess->listener && sess->listener->counters) _HA_ATOMIC_ADD(&sess->listener->counters->bytes_in, bytes); @@ -844,7 +844,7 @@ void stream_process_counters(struct stream *s) _HA_ATOMIC_ADD(&s->be->be_counters.bytes_out, bytes); if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.bytes_out, bytes); + _HA_ATOMIC_ADD(&__objt_server(s->target)->counters.bytes_out, bytes); if (sess->listener && sess->listener->counters) _HA_ATOMIC_ADD(&sess->listener->counters->bytes_out, bytes); @@ -916,7 +916,7 @@ static void back_establish(struct stream *s) } if (objt_server(s->target)) - health_adjust(objt_server(s->target), HANA_STATUS_L4_OK); + health_adjust(__objt_server(s->target), HANA_STATUS_L4_OK); if (!IS_HTX_STRM(s)) { /* let's allow immediate data connection in this case */ /* if the user wants to log as soon as possible, without counting @@ -1439,7 +1439,7 @@ static int process_store_rules(struct stream *s, struct channel *rep, int an_bit struct dict_entry *de; struct stktable *t = s->store[i].table; - if (objt_server(s->target) && objt_server(s->target)->flags & SRV_F_NON_STICK) { + if (objt_server(s->target) && __objt_server(s->target)->flags & SRV_F_NON_STICK) { stksess_free(s->store[i].table, s->store[i].ts); s->store[i].ts = NULL; continue; @@ -2428,8 +2428,8 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) si_b->prev_state == SI_ST_EST) { chunk_printf(&trash, "%08x:%s.srvcls[%04x:%04x]\n", s->uniq_id, s->be->id, - objt_cs(si_f->end) ? (unsigned short)objt_cs(si_f->end)->conn->handle.fd : -1, - objt_cs(si_b->end) ? (unsigned short)objt_cs(si_b->end)->conn->handle.fd : -1); + objt_cs(si_f->end) ? (unsigned short)__objt_cs(si_f->end)->conn->handle.fd : -1, + objt_cs(si_b->end) ? (unsigned short)__objt_cs(si_b->end)->conn->handle.fd : -1); DISGUISE(write(1, trash.area, trash.data)); } @@ -2437,8 +2437,8 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) si_f->prev_state == SI_ST_EST) { chunk_printf(&trash, "%08x:%s.clicls[%04x:%04x]\n", s->uniq_id, s->be->id, - objt_cs(si_f->end) ? (unsigned short)objt_cs(si_f->end)->conn->handle.fd : -1, - objt_cs(si_b->end) ? (unsigned short)objt_cs(si_b->end)->conn->handle.fd : -1); + objt_cs(si_f->end) ? (unsigned short)__objt_cs(si_f->end)->conn->handle.fd : -1, + objt_cs(si_b->end) ? (unsigned short)__objt_cs(si_b->end)->conn->handle.fd : -1); DISGUISE(write(1, trash.area, trash.data)); } } @@ -2505,8 +2505,8 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) (!(global.mode & MODE_QUIET) || (global.mode & MODE_VERBOSE)))) { chunk_printf(&trash, "%08x:%s.closed[%04x:%04x]\n", s->uniq_id, s->be->id, - objt_cs(si_f->end) ? (unsigned short)objt_cs(si_f->end)->conn->handle.fd : -1, - objt_cs(si_b->end) ? (unsigned short)objt_cs(si_b->end)->conn->handle.fd : -1); + objt_cs(si_f->end) ? (unsigned short)__objt_cs(si_f->end)->conn->handle.fd : -1, + objt_cs(si_b->end) ? (unsigned short)__objt_cs(si_b->end)->conn->handle.fd : -1); DISGUISE(write(1, trash.area, trash.data)); } @@ -3213,8 +3213,8 @@ static int stats_dump_full_strm_to_buffer(struct stream_interface *si, struct st if (strm->be->cap & PR_CAP_BE) chunk_appendf(&trash, " server=%s (id=%u)", - objt_server(strm->target) ? objt_server(strm->target)->id : "", - objt_server(strm->target) ? objt_server(strm->target)->puid : 0); + objt_server(strm->target) ? __objt_server(strm->target)->id : "", + objt_server(strm->target) ? __objt_server(strm->target)->puid : 0); else chunk_appendf(&trash, " server= (id=-1)"); @@ -3590,7 +3590,7 @@ static int cli_io_handler_dump_sess(struct appctx *appctx) get_host_port(conn->src), strm_fe(curr_strm)->id, (curr_strm->be->cap & PR_CAP_BE) ? curr_strm->be->id : "", - objt_server(curr_strm->target) ? objt_server(curr_strm->target)->id : "" + objt_server(curr_strm->target) ? __objt_server(curr_strm->target)->id : "" ); break; case AF_UNIX: @@ -3599,7 +3599,7 @@ static int cli_io_handler_dump_sess(struct appctx *appctx) strm_li(curr_strm)->luid, strm_fe(curr_strm)->id, (curr_strm->be->cap & PR_CAP_BE) ? curr_strm->be->id : "", - objt_server(curr_strm->target) ? objt_server(curr_strm->target)->id : "" + objt_server(curr_strm->target) ? __objt_server(curr_strm->target)->id : "" ); break; } diff --git a/src/stream_interface.c b/src/stream_interface.c index 8ad1854970..b78c3bc48e 100644 --- a/src/stream_interface.c +++ b/src/stream_interface.c @@ -434,7 +434,7 @@ static void stream_int_notify(struct stream_interface *si) /* process consumer side */ if (channel_is_empty(oc)) { - struct connection *conn = objt_cs(si->end) ? objt_cs(si->end)->conn : NULL; + struct connection *conn = objt_cs(si->end) ? __objt_cs(si->end)->conn : NULL; if (((oc->flags & (CF_SHUTW|CF_SHUTW_NOW)) == CF_SHUTW_NOW) && (si->state == SI_ST_EST) && (!conn || !(conn->flags & (CO_FL_WAIT_XPRT | CO_FL_EARLY_SSL_HS))))