From: Claudio Jeker Subject: Re: httpd: fix scan-build dead stores findings To: Rafael Sadowski Cc: Theo Buehler , tech@openbsd.org Date: Fri, 2 Jan 2026 22:57:14 +0100 On Fri, Jan 02, 2026 at 09:51:52PM +0100, Rafael Sadowski wrote: > On Fri Jan 02, 2026 at 08:48:09PM +0100, Theo Buehler wrote: > > On Fri, Jan 02, 2026 at 08:14:29PM +0100, Rafael Sadowski wrote: > > > On Thu Jan 01, 2026 at 10:12:57AM +0100, Stefan Sperling wrote: > > > > On Sun, Dec 28, 2025 at 06:55:28AM +0100, Rafael Sadowski wrote: > > > > > Hi, > > > > > > > > > > scan-build flagged a few dead stores in httpd: > > > > > > > > > > - config.c:636 – s incremented but never used after > > > > > - httpd.c:533 – q assigned but immediately overwritten in the loop > > > > > - server.c:891 – inrd/inwr assigned, then reassigned, never actually needed > > > > > - server_fcgi.c:678,690 – kv result unused > > > > > > > > > > OK? > > > > > > > > > > Diff removes all of them. OK? > > > > > 1. src/usr.sbin/httpd/config.c:636:3: warning: Value stored to 's' is never read [deadcode.DeadStores] > > > > > 636 | s += srv_conf->return_uri_len; > > > > > > > > > > 2. src/usr.sbin/httpd/httpd.c:533:6: warning: Although the value stored to 'q' is used in the enclosing expression, the value is never actually read from 'q' [deadcode.DeadStores] > > > > > 533 | p = q = label; > > > > > | ^ ~~~~~ > > > > > > > > > > 3. src/usr.sbin/httpd/server.c:891:15: warning: Value stored to 'inrd' during its initialization is never read [deadcode.DeadStores] > > > > > 891 | evbuffercb inrd = server_read; > > > > > | ^~~~ ~~~~~~~~~~~ > > > > > > > > > > 4. src/usr.sbin/httpd/server_fcgi.c:678:9: warning: Although the value stored to 'kv' is used in the enclosing expression, the value is never actually read from 'kv' [deadcode.DeadStores] > > > > > 678 | if ((kv = kv_find(&resp->http_headers, &key)) == NULL) { > > > > > | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > src/usr.sbin/httpd/server_fcgi.c:690:8: warning: Although the value stored to 'kv' is used in the enclosing expression, the value is never actually read from 'kv' [deadcode.DeadStores] > > > > > 690 | if ((kv = kv_find(&resp->http_headers, &key)) != NULL) { > > > > > | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > > diff --git a/usr.sbin/httpd/config.c b/usr.sbin/httpd/config.c > > > > > index b45081129b7..aa2cd382ed3 100644 > > > > > --- a/usr.sbin/httpd/config.c > > > > > +++ b/usr.sbin/httpd/config.c > > > > > @@ -531,7 +531,6 @@ config_getserver_config(struct httpd *env, struct server *srv, > > > > > if ((srv_conf->return_uri = get_data(p + s, > > > > > srv_conf->return_uri_len)) == NULL) > > > > > goto fail; > > > > > - s += srv_conf->return_uri_len; > > > > > > > > I am not 100% sure about this one. On one hand scan-build is right that the > > > > value of s is never used in the remainder of this function. On the other > > > > hand, this code seems to be written with the assumption in mind that more > > > > optional chuks of data with variable length could be added in the future. > > > > If we ever grow the server config structure and process more data via the > > > > 'p + s' idiom then this line would need to be added back in to avoid a bug. > > > > So perhaps this line is worth keeping? > > > > > > > > The rest is ok stsp@ > > > > > > Thanks Stefan, > > > > > > I've committed the rest of the dead store fixes but left this one out for now. > > > > > > I see your point about future expansion, but I'd lean towards removing > > > it. The comment already documents that more variable-length fields could > > > be added. If someone later adds TLS data or other fields, they will need > > > to modify this function anyway. > > > > I think the more important thing to fix here is that there is no check > > whether the imsg contains enough data to copy reurn_uri_len bytes from > > p + s. > > Thanks Theo! That's right, there is no check as in config_getserver. > Tested with regress/usr.sbin/httpd. > > diff --git a/usr.sbin/httpd/config.c b/usr.sbin/httpd/config.c > index b45081129b7..7d41b57305f 100644 > --- a/usr.sbin/httpd/config.c > +++ b/usr.sbin/httpd/config.c > @@ -513,6 +513,11 @@ config_getserver_config(struct httpd *env, struct server *srv, > /* Reset these variables to avoid free'ing invalid pointers */ > serverconfig_reset(srv_conf); > > + if ((IMSG_DATA_SIZE(imsg) - s) < (size_t)srv_conf->return_uri_len) { > + log_debug("%s: invalid message length", __func__); > + goto fail; > + } > + > TAILQ_FOREACH(parent, &srv->srv_hosts, entry) { > if (strcmp(parent->name, srv_conf->name) == 0) > break; > @@ -531,7 +536,6 @@ config_getserver_config(struct httpd *env, struct server *srv, > if ((srv_conf->return_uri = get_data(p + s, > srv_conf->return_uri_len)) == NULL) > goto fail; > - s += srv_conf->return_uri_len; > } > > if (srv_conf->flags & SRVFLAG_LOCATION) { > It would be much nicer to use the real ibuf apis and not use direct imsg data access, then all those length checks can go. Also IMSG_DATA_SIZE is spelled imsg_get_len(). -- :wq Claudio