Re: [PATCH v1 1/1] arm64: Fix strrchr() matching of null terminator

["Edgar E. Iglesias" <edgar.iglesias@xxxxxxx>]

On Tue, May 19, 2026 at 08:40:54AM +0200, Jan Beulich wrote:
> On 19.05.2026 01:43, Edgar E. Iglesias wrote:
> > The generic Xen strrchr() implementation returns a pointer to the string
> > terminator when searching for '\0', matching the standard C semantics.
> > 
> > The ARM64 assembly version stopped as soon as it loaded the terminator and
> > returned the previous match pointer instead.  This made strrchr("", '\0')
> > return NULL.
> 
> I wonder though: Why would one pass '\0' to strrchr()? If you want to find
> the end of a string, more efficient (at least in the general case) options
> exist (strchr(), memchr(), strlen()).

Right, this came from a fuzzer checking standard strrchr() behavior, not
from an explicit end-of-string use case.

I agree strlen() or strchr(s, '\0') would be better for that, but
strrchr(s, '\0') is valid input, so we should handle it to avoid
surprises for existing or future callers.


> 
> > Compare the loaded byte against the requested character before deciding
> > whether to stop at the terminator, so the terminator itself can be returned
> > when it is the requested character.
> 
> Nit: "..., so a pointer to the terminator ...".

Updated, thanks.


> 
> > Fixes: 42c4eb6a83 ("xen: arm64: assembly optimised mem* and str*")
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> However, the function having come from Linux, imo the patch wants to go to
> Linux (ideally first, but at the very least also).

Yes, sounds good.


> 
> Additionally, looking at strchr() - couldn't the code here be written in a
> similar way, allowing to get away with just a single branch? (Arm32's pair
> of functions is also pretty similar in this regard.)

Hm, good point. I had a closer look and came up with a couple of options
that look better. This one is not quite the same shape as strchr(), but it
eliminates one branch and shortens the inner loop from 5 to 4 insns...

FUNC(strrchr)
        mov     x3, #0          /* Last match, or NULL */
        sub     x0, x0, #1      /* For the pre-indexed load */
        and     w1, w1, #0xff
1:      ldrb    w2, [x0, #1]!   /* Load next byte */
        cmp     w2, w1
        csel    x3, x0, x3, eq  /* Remember match */
        cbnz    w2, 1b          /* Keep going until NUL */
        mov     x0, x3
        ret
END(strrchr)

Cheers,
Edgar

> 
> Jan
> 
> > --- a/xen/arch/arm/arm64/lib/strrchr.S
> > +++ b/xen/arch/arm/arm64/lib/strrchr.S
> > @@ -30,11 +30,10 @@ FUNC(strrchr)
> >     mov     x3, #0
> >     and     w1, w1, #0xff
> >  1: ldrb    w2, [x0], #1
> > -   cbz     w2, 2f
> >     cmp     w2, w1
> > -   b.ne    1b
> > +   b.ne    2f
> >     sub     x3, x0, #1
> > -   b       1b
> > -2: mov     x0, x3
> > +2: cbnz    w2, 1b
> > +   mov     x0, x3
> >     ret
> >  END(strrchr)
>