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.

However, I don't really have a strong opinion on this either.