On Thu, Jul 16, 2026 at 09:36:47AM +0200, Rafael Sadowski wrote:
> On Thu Jul 16, 2026 at 08:50:21AM +0200, Claudio Jeker wrote:
> > On Thu, Jul 16, 2026 at 07:42:36AM +0200, Rafael Sadowski wrote:
> > > On Wed Jul 15, 2026 at 08:56:27AM +0200, Claudio Jeker wrote:
> > > > On Wed, Jul 15, 2026 at 08:08:51AM +0200, Rafael Sadowski wrote:
> > > > > On Mon Jul 13, 2026 at 02:16:58AM +0200, Kirill A. Korinsky wrote:
> > > > > > On Sat, 11 Jul 2026 16:12:46 +0200,
> > > > > > Rafael Sadowski <rafael@sizeofvoid.org> wrote:
> > > > > > > 
> > > > > > > I think this is a solid first version for that feature. We'll certainly
> > > > > > > find out about more edge cases through user feedback ... or maybe not ;)
> > > > > > > 
> > > > > > 
> > > > > > Here first pass for review. I haven't spent much time on this. I may be wrong.
> > > > > > 
> > > > > > > +	TAILQ_FOREACH(hdr, &parent_srv->srv_conf.headers, entry) {
> > > > > > > +		if (header_exists(srv_conf, hdr->name)) {
> > > > > > > +			DPRINTF("%s: skipping header \"%s\" from parent "
> > > > > > > +			    "\"%s\", overridden in location \"%s\"",
> > > > > > > +			    __func__, hdr->name,
> > > > > > > +			    parent_srv->srv_conf.name, srv_conf->location);
> > > > > > > +			continue;
> > > > > > > +		}
> > > > > > 
> > > > > > What happens with duplicated headers? Multiple Set-Cookie is good example.
> > > > > 
> > > > > Good catch!
> > > > > 
> > > > > Fixed by collecting the inherited headers in a temporary list and
> > > > > TAILQ_CONCAT()ing them afterwards, so config_header_exists() only ever sees
> > > > > headers defined in the location itself.
> > > > > 
> > > > > The semantics are unchanged otherwise: if a location mentions a header name at
> > > > > all, it takes over that name completely and none of the parent's headers
> > > > > with that name are inherited. So a "header add "Set-Cookie"" in a
> > > > > location replaces the whole parent group rather than adding to it. The
> > > > > users can use remove if they want to drop individual inherited headers.
> > > > > Documented in httpd.conf(5).
> > > > > 
> > > > > > 
> > > > > > > @@ -1050,6 +1063,7 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
> > > > > > >  	free(hstsheader);
> > > > > > >  	free(clenheader);
> > > > > > >  	free(bannerheader);
> > > > > > > +	free(customheaders);
> > > > > > 
> > > > > > I think this code is reached only on error, and it makes customheaders leaks
> > > > > > on sucess.
> > > > > > 
> > > > > > And next things which I not yet sure. I think that roughly 25 directives
> > > > > > exceed the 16K imsg limit because every one is fixed 664 byte structure nad
> > > > > > it is packed into one message.
> > > > > 
> > > > > So true! That's what happens when you follow the code from fastcgi.
> > > > > FastCGI has the same problem. It's actually even worse:
> > > > 
> > > > With imsg_set_maxsize(3) you can alter the limit. e.g. bgpd uses a 128k
> > > > imsg size. You can go quite large but keep in mind that the full message
> > > > needs to a) fit in memory and b) needs to be transferred before other
> > > > messages can be processed (the imsgq is a fifo).
> > > >  
> > > > >  #define HTTPD_FCGI_NAME_MAX    511
> > > > >  #define HTTPD_FCGI_VAL_MAX     511
> > > > > 
> > > > > I fixed the problem by sending the headers one after the other.
> > > > 
> > > > Isn't 511 bytes for a header a bit short? e.g. some cookies and JWT tokens
> > > > can be rather large.
> > > 
> > > Yes that's true. I upper the limits for header to 256/8192 and not to
> > > touch the IMSG limit for the time being.
> > > 
> > > > 
> > > > The imsg passing in config_setserver_headers() sends the tailq pointers
> > > > which breaks the fork/exec ASLR protection. I would suggest to use encode
> > > > the headers differently and not use the struct for passing. With that also
> > > > the fixed size string buffers can be changed to a length + str value
> > > > encoding.
> > > > 
> > > > You can probably shoehorn this into proc_composev, on the receive side
> > > > just use imsg_get_buf() and imsg_get_strbuf() to pull the data out.
> > > >  
> > > 
> > > Ohh! Thanks, fixed like this:
> > > 
> > > config_setserver_headers() now sends a small fixed struct header_imsg
> > > (id, flags, and the name/value lengths) followed by the raw name and
> > > value bytes via proc_composev()
> > > 
> > > As you suggested, the length encoding also let me drop the fixed
> > > name[]/value[] buffers in custom_header.
> > > 
> > > If you only interested in review this part, you can find it here
> > > https://rsadowski.gothub.org/?action=diff&commit=daa3667f40735c225a6ba94ebd6bb7bd90a115bc&headref=custom-http-header-support&path=httpd.git
> > > 
> > > If we can agree on that, I can and will implement ("fix") it for FastCGI
> > > as well.
> > > 
> > > Here is the full diff and another iteration with the following changes:
> > > 
> > >     httpd: apply "add ... always" headers to internal error responses
> > > 
> > >     Bring back get_always_custom_headers() to append "add ... always"
> > >     headers to those responses. Only "add" is handled here. "set" and
> > >     "remove" are ignored for now, so "set ... always" is dropped from the
> > >     grammar. Framing headers are already rejected at parse time, so a raw
> > >     append cannot desync the response, only produce a duplicate header.
> > > 
> > >     This is a stopgap. A later change should fold this into
> > >     server_custom_headers() so all header operations work the same way on
> > >     every response.
> > > 
> > > That should be "everything",
> > 
> > Looks good to me. Two minor comments.
> > 
> > - why is always only supported for add?
> >   Why can't I remove or set headers in error conditions?
> 
> Because the error/abort path doesn't build its response through the
> kvtree like normal responses do. It formats the headers as a raw string.
> In that path "add" is the only operation that can be applied "safely".
> "set" and "remove" would need to modify the built-in headers.
> 
> Rather than half-implementing set/remove on a second code path, I
> limited "always" to "add" for now and marked it XXX. The plan is to
> convert server_abort_http() to build its headers via the kvtree too,
> then all three operations work the same on every response and the
> restriction goes away.
 
Why can't we use the normal path here? Sure if httpd is out of memory
kvtree ops will fail but so will most bufferevent calls etc.
One could still fall back to static headers then...

Anyway, you answered my question and the reason is the usual httpd horror.

-- 
:wq Claudio