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

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

[Edgar E. Iglesias]

Found a problem with strrchr("", '\0') returning NULL when searching for the
terminating null byte. This fixes it.

Found while GDB-fuzzing xen/lib helpers under QEMU.

Cheers,
Edgar

Edgar E. Iglesias (1):
  arm64: Fix strrchr() matching of null terminator

 xen/arch/arm/arm64/lib/strrchr.S | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
2.43.0

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

[Edgar E. Iglesias]

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.

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.

Fixes: 42c4eb6a83 ("xen: arm64: assembly optimised mem* and str*")
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 xen/arch/arm/arm64/lib/strrchr.S | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm64/lib/strrchr.S b/xen/arch/arm/arm64/lib/strrchr.S
index 81033c0822..31f304b183 100644
--- 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)
-- 
2.43.0

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

[Jan Beulich]

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()).

> 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 ...".

> 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).

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.)

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)

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

[Julien Grall]

Hi Edgar and Jan,

On 19/05/2026 07:40, 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()).

+1 I am interested to know the use-case for this change. Is this for 
compliance or real issue? If the latter, can we add some details.

It might also be worth to write a selftest to avoid any regression (in 
particular if we decide to diverge from Linux).

> 
>> 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 ...".
> 
>> 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).

We are trying to keep the core implementation in lib the same as linux 
(see arch/arm/README.LinuxPrimitives). I would prefer if this is also 
first committed to Linux and then backported.

Cheers,

-- 
Julien Grall

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

[Andrew Cooper]

On 19/05/2026 12:43 am, 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.
>
> 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.
>
> Fixes: 42c4eb6a83 ("xen: arm64: assembly optimised mem* and str*")
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  xen/arch/arm/arm64/lib/strrchr.S | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

A couple of tangential things.

This file was inherited from Linux.  Does the same bug need fixing
there?  What about the arm32 side?

Looking at your example, it surely wasn't actually as simple as
strrchr("", '\0') ?  I'd expect the optimiser to be able to turn that
into a constant and not call out to the library implementation.

Elsewhere, I've created xen/common/bitops.c to be CONFIG_SELF_TESTS for
the bit operations including the arch-optimised variations, because
they're subtle and easy to get wrong.  This looks like it's worth doing
the same for the bits of libc we implement.

~Andrew

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

[Edgar E. Iglesias]

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)
> 

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

[Edgar E. Iglesias]

On Tue, May 19, 2026 at 08:47:17AM +0100, Julien Grall wrote:
> Hi Edgar and Jan,
> 
> On 19/05/2026 07:40, 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()).
> 
> +1 I am interested to know the use-case for this change. Is this for
> compliance or real issue? If the latter, can we add some details.
> 
> It might also be worth to write a selftest to avoid any regression (in
> particular if we decide to diverge from Linux).
> 
> > 
> > > 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 ...".
> > 
> > > 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).
> 
> We are trying to keep the core implementation in lib the same as linux (see
> arch/arm/README.LinuxPrimitives). I would prefer if this is also first
> committed to Linux and then backported.
>

Sounds good, thanks!

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

[Edgar E. Iglesias]

On Tue, May 19, 2026 at 01:45:37PM +0100, Andrew Cooper wrote:
> On 19/05/2026 12:43 am, 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.
> >
> > 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.
> >
> > Fixes: 42c4eb6a83 ("xen: arm64: assembly optimised mem* and str*")
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > ---
> >  xen/arch/arm/arm64/lib/strrchr.S | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> A couple of tangential things.
> 
> This file was inherited from Linux.  Does the same bug need fixing
> there?  What about the arm32 side?

Yes, I'll have a look at Linux. Arm32 looks fine to me.

> 
> Looking at your example, it surely wasn't actually as simple as
> strrchr("", '\0') ?  I'd expect the optimiser to be able to turn that
> into a constant and not call out to the library implementation.
> 

We ran into this while exploring a new QEMU based test framework, we had
it do some fuzzing and it tripped over this.


> Elsewhere, I've created xen/common/bitops.c to be CONFIG_SELF_TESTS for
> the bit operations including the arch-optimised variations, because
> they're subtle and easy to get wrong.  This looks like it's worth doing
> the same for the bits of libc we implement.
>

Yes, that looks like a good fit, thanks.

Cheers,
Edgar