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

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

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@amd.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> 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)
>