Hi Mateusz,
On Tue, Aug 16, 2022 at 11:58:02PM +0000, hapr...@sl.damisa.net wrote:
> Hi,
>
> as suggested by Willy on GitHub, I'm submitting my patch for
> https://github.com/haproxy/haproxy/issues/1822.
Thank you!
> This is my first contribution, so I'm tagging it as RFC for now ;)
>
> I'm not entirely happy with using goto (suggested by Tim) to avoid
> hitting htx_get_next_blk call at the end of the loop, but I'm not
> familiar with HAproxy coding standards.
We're generally fine with gotos when they're the most efficient
solution (anyway, 1/5 of instructions executed by a CPU are jumps,
so it's pointless to hate gotos, it's just that using too many of
them will make the code less readable).
However here the goto at the beginning potentially maintains a
problem: I'm wondering how we can be certain that htx_remove_blk()
doesn't return a NULL if we remove the last block, in which case
going back to the beginning of the loop without testing it can
be a problem.
> I think it would be nicer to:
>
> 1. Introduce flag variable preserve_blk
> 2. Reset it to 1 at the beginning of the while (blk) loop
> 3. Replace htx_remove_blk + continue with preserve_blk = 0 + break
> 4. At the end of the loop, call htx_get_next_blk if preserve_blk is set
> or call htx_remove_blk otherwise
Yep I agree with this. I was thinking that an even better option
would be to remove the branches from the inner loop and use it only
to spot unusual chars, something like:
end = istlen(n);
for (i = 0; i < end; i++) {
if (!isalnum() ...)
break;
}
if (i < end) {
/* unwanted char found */
if (px->options...)
goto block;
blk = htx_remove_blk();
continue;
}
The for() loop could even be replaced with strcspn(). I just suspect
that for large sets it's slower if it needs to rebuild an internal map,
so we'd probably rather keep the for() loop as-is.
> I have not included severity of the patch, because on GitHub issue is
> still marked as `status: needs-triage`. I think MEDIUM would be
> appropriate.
Yes I think so as well.
> By the way, while writing VTest to cover this bug, I spotted something
> "suspicious" about reg tests for FCGI backends - my-fcgi-app FCGI app is
> defined, but it is not used anywhere? be-fcgi* backends look exactly
> like be-http* to me.
Indeed, I don't know either. I'm wondering if it's not needed just to
enable something by just being present, but I don't see what (and no
comment in the file indicates this). Maybe it's a leftover of an
initial attempt at testing fcgi, I don't know. Let's keep it for now,
we'll check with Christopher when he's back.
But if you can provide an updated patch for the loop, I'm fine with
merging your patch as-is.
Thanks!
Willy
