On Wed, Aug 31, 2022 at 06:25:01PM +0200, Omar Polo wrote:
> On 2022/08/31 11:40:44 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > @@ -278,6 +350,13 @@ render:
> >  
> >  	goto done;
> >  err:
> > +	if (srv != NULL) {
> > +		/*
> > +		 * We might have run into problems due to stale repository
> > +		 * caches. Close all repositories to try again later.
> > +		 */
> > +		invalidate_repo_cache(srv);
> 
> We `goto err' also when fcgi_printf returns -1, i.e. upon premature
> client disconnection.  (OK, also if asprintf fails, but that error is
> not propagate up to here and it's not relevant for the cache anyway.)
> I think it should look if error is non-NULL.
>
> There is another issue that I think is worth being raised: clients can
> force errors by playing with the query string.  By passing invalid
> query parameters they can force all kinds of erros being returned here
> (from no git directory, to no object, to no file entry...) and so
> making us invalidate the cache.

You're right, always invalidating on any error is not ideal,
especially when it can be triggered via a bad query string.

I will think some more about this, and commit the previous
version of the diff, for now. It might be possible to fix this
issue in the lib/ code where relevant errors are being raised.