[PATCH] s390x: Fix segfault in wcsncmp [BZ #31934]

[Stefan Liebler]

On 11.07.24 14:57, Carlos O'Donell wrote:
> On 7/11/24 5:28 AM, Stefan Liebler wrote:
>> The z13/vector-optimized wcsncmp implementation segfaults if n=1
>> and there is only one character (equal on both strings) before
>> the page end.  Then it loads and compares one character and misses
>> to check n again.  The following load fails.
> 
> LGTM. Thank you for fixing this!
> 
> Please feel free to push since we're currently in bug fixing phase and this has
> no ABI impact.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks for reviewing.
I've just committed it.

Note that the wcsncmp implementation was introduced in glibc 2.23:
https://sourceware.org/git/?p=glibc.git;a=commit;h=cee82e70ccb7b2f054cd781b0a603ae244523e72
'S390: Optimize strncmp and wcsncmp.'

Can you please advice which release-branches are maintained and to which
one I should backport them?
To all from glibc 2.39 down to 2.23?

Thanks,
Stefan
> 
>> This patch removes the extra load and compare of the first character
>> and just start with the loop which uses vector-load-to-block-boundary.
>> This code-path also checks n.
>>
>> With this patch both tests are passing:
>> - the simplified one mentioned in the bugzilla 31934
>> - the full one in Florian Weimer's patch:
>> "manual: Document a GNU extension for strncmp/wcsncmp"
>> (https://patchwork.sourceware.org/project/glibc/patch/874j9eml6y.fsf@oldenburg.str.redhat.com/):
>> On s390x-linux-gnu (z16), the new wcsncmp test fails due to bug 31934.
>> ---
>>  sysdeps/s390/wcsncmp-vx.S | 10 +---------
>>  1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/sysdeps/s390/wcsncmp-vx.S b/sysdeps/s390/wcsncmp-vx.S
>> index bf6dfa6bc2..8b081567a2 100644
>> --- a/sysdeps/s390/wcsncmp-vx.S
>> +++ b/sysdeps/s390/wcsncmp-vx.S
>> @@ -59,14 +59,7 @@ ENTRY(WCSNCMP_Z13)
>>  	sllg	%r4,%r4,2	/* Convert character-count to byte-count.  */
>>  	locgrne	%r4,%r1		/* Use max byte-count, if bit 0/1 was one.  */
>>  
>> -	/* Check first character without vector load.  */
>> -	lghi	%r5,4		/* current_len = 4 bytes.  */
>> -	/* Check s1/2[0].  */
>> -	lt	%r0,0(%r2)
>> -	l	%r1,0(%r3)
>> -	je	.Lend_cmp_one_char
>> -	crjne	%r0,%r1,.Lend_cmp_one_char
> 
> OK. Removes the unrolled processing of just 1 character.
> 
>> -
>> +	lghi	%r5,0		/* current_len = 0 bytes.  */
> 
> OK. Current progress is set to 0 at the start (as expected).
> 
>>  .Lloop:
>>  	vlbb	%v17,0(%r5,%r3),6 /* Load s2 to block boundary.  */
>>  	vlbb	%v16,0(%r5,%r2),6 /* Load s1 to block boundary.  */
> 
> ... and we use vlbb to avoid crossing the block boundary (page boundary).
> 
>> @@ -167,7 +160,6 @@ ENTRY(WCSNCMP_Z13)
>>  	srl	%r4,2		/* And convert it to character-index.  */
>>  	vlgvf	%r0,%v16,0(%r4)	/* Load character-values.  */
>>  	vlgvf	%r1,%v17,0(%r4)
>> -.Lend_cmp_one_char:
> 
> OK. Label no longer needed.
> 
>>  	cr	%r0,%r1
>>  	je	.Lend_equal
>>  	lghi	%r2,1
>