On Thu, Aug 8, 2024 at 10:19 AM Ruediger Pluem <rpl...@apache.org> wrote:
Sorry for the late reply, I was quite busy at work$ back from vacations..
>
> On 7/17/24 10:50 PM, yla...@apache.org wrote:
> > --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> > +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Wed Jul 17 20:50:12 2024
> >
> > + span = strcspn(input, EXPAND_SPECIALS "?");
> > + if (input[span] == '?') {
> > + /* this qmark is not from an expansion thus safe */
> > + *unsafe_qmark = 0;
> > +
> > + /* keep tracking only if interested in the last qmark */
> > + if (entry && (entry->flags & RULEFLAG_QSLAST)) {
> > + do {
> > + span++;
> > + span += strcspn(input + span, EXPAND_SPECIALS "?");
> > + } while (input[span] == '?');
>
> Why do we need this loop? Isn't that the same as
>
> span += strcspn(input + span, EXPAND_SPECIALS);
Yes correct, I first thought of returning the offset of the qmark but
a bool is enough, didn't change the loop though.
Changed both here and below to what you propose, in r1920566.
>
> > + }
> > + else {
> > + unsafe_qmark = NULL;
> > + span += strcspn(input + span, EXPAND_SPECIALS);
> > + }
> > + }
> > + }
> > +
> > /* check the remainder */
> > - if (*p && (span = strcspn(p, "\\$%")) > 0) {
>
> Can't we just do a break here in the !*p case?
We could, but we are not on the fast path here and I don't think one
more strcspn() on an empty string is going to hurt compared to the
expansion work. We could break out earlier in other places in this
loop too, not worth complicating the code imho.
Thanks for reviewing!
Regards;
Yann.
