Bug 430354 - ppc stxsibx and stxsihx instructions write too much data
Summary: ppc stxsibx and stxsihx instructions write too much data
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (other bugs)
Version First Reported In: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-13 22:01 UTC by Mark Wielaard
Modified: 2020-12-15 11:00 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments
Implement stxsibx and stxsihx as direct byte or half-word store (2.42 KB, text/plain)
2020-12-13 22:06 UTC, Mark Wielaard
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2020-12-13 22:01:02 UTC
On a system with a power9 optimized glibc memcheck/tests/str_tester (and any other application using some of the glibc string functions) fail with:

==473431== Invalid write of size 8
==473431==    at 0x419C4CC: strndup (strndup.c:48)
==473431==    by 0x1000B08F: test_strndup (str_tester.c:1297)
==473431==    by 0x1000C043: main (str_tester.c:1528)
==473431==  Address 0x4300046 is 6 bytes inside a block of size 7 alloc'd
==473431==    at 0x40A428C: malloc (vg_replace_malloc.c:307)
==473431==    by 0x419C4B3: strndup (strndup.c:43)
==473431==    by 0x1000B08F: test_strndup (str_tester.c:1297)
==473431==    by 0x1000C043: main (str_tester.c:1528)

Looking with gdb finds:

0x000000000419c4cc in __GI___strndup (s=0x1000c418 "abcdef", n=<optimized out>)
    at strndup.c:48
48	  new[len] = '\0';

Dump of assembler code for function __GI___strndup:
   0x000000000419c480 <+0>:	addis   r2,r12,22
   0x000000000419c484 <+4>:	addi    r2,r2,-20864
   0x000000000419c488 <+8>:	mflr    r0
   0x000000000419c48c <+12>:	std     r30,-16(r1)
   0x000000000419c490 <+16>:	std     r31,-8(r1)
   0x000000000419c494 <+20>:	std     r0,16(r1)
   0x000000000419c498 <+24>:	stdu    r1,-48(r1)
   0x000000000419c49c <+28>:	mr      r30,r3
   0x000000000419c4a0 <+32>:	bl      0x41ae390 <__strnlen_ppc>
   0x000000000419c4a4 <+36>:	nop
   0x000000000419c4a8 <+40>:	mr      r31,r3
   0x000000000419c4ac <+44>:	addi    r3,r3,1
   0x000000000419c4b0 <+48>:	bl      0x4118cc0 <00000025.plt_call.malloc>
   0x000000000419c4b4 <+52>:	ld      r2,24(r1)
   0x000000000419c4b8 <+56>:	mr.     r9,r3
   0x000000000419c4bc <+60>:	beq     0x419c4dc <__GI___strndup+92>
   0x000000000419c4c0 <+64>:	xxspltib vs0,0
   0x000000000419c4c4 <+68>:	mr      r4,r30
   0x000000000419c4c8 <+72>:	mr      r5,r31
=> 0x000000000419c4cc <+76>:	stxsibx vs0,r9,r31
   0x000000000419c4d0 <+80>:	bl      0x4118c80 <00000025.plt_call.__GI_memcpy>
   0x000000000419c4d4 <+84>:	ld      r2,24(r1)
   0x000000000419c4d8 <+88>:	mr      r9,r3
   0x000000000419c4dc <+92>:	addi    r1,r1,48
   0x000000000419c4e0 <+96>:	mr      r3,r9
   0x000000000419c4e4 <+100>:	ld      r0,16(r1)
   0x000000000419c4e8 <+104>:	ld      r30,-16(r1)
   0x000000000419c4ec <+108>:	ld      r31,-8(r1)
   0x000000000419c4f0 <+112>:	mtlr    r0
   0x000000000419c4f4 <+116>:	blr
   0x000000000419c4f8 <+120>:	.long 0x0
   0x000000000419c4fc <+124>:	.long 0x1000000
   0x000000000419c500 <+128>:	.long 0x280
End of assembler dump.

So it is using stxsibx to write one byte at the end of the string.

But stxsibx is implemented by first reading a whole word, merging in the new byte, and then writing out the whole word. Causing memcheck to warn the wrote is beyond the end of (malloced) string buffer.

The the stxsihx (Store VSX Scalar as Integer Halfword Indexed X-form) instruction does something similar.

It isn't immediately clear why the code isn't just storing the byte directly, IRStmt_Store seems fine to store byte or half word sized data, and so seems the ppc backend.
Comment 1 Mark Wielaard 2020-12-13 22:06:01 UTC
Created attachment 134060 [details]
Implement stxsibx and stxsihx as direct byte or half-word store

This implements stxsibx and stxsihx as direct byte or half-word stores of 8 or 16 bytes without first reading the whole word at the effective address. This fixes the issue with memcheck/tests/str_tester and some other string functions and doesn't seem to produce any regressions.
Comment 2 Carl Love 2020-12-14 21:23:10 UTC
The attached patch looks good to me.
Comment 3 Mark Wielaard 2020-12-15 11:00:10 UTC
commit ab257bc49a6c8beefa794470446f917ec441f718
Author: Mark Wielaard <mark@klomp.org>
Date:   Tue Dec 15 11:49:58 2020 +0100

    ppc stxsibx and stxsihx instructions write too much data
    
    stxsibx (Store VSX Scalar as Integer Byte Indexed X-form) is implemented
    by first reading a whole word, merging in the new byte, and then writing
    out the whole word. Causing memcheck to warn when the destination might
    have room for less than 8 bytes.
    
    The stxsihx (Store VSX Scalar as Integer Halfword Indexed X-form)
    instruction does something similar reading and then writing a full
    word instead of a half word.
    
    The code can be simplified (and made more correct) by storing the byte
    (or half-word) directly, IRStmt_Store seems fine to store byte or half
    word sized data, and so seems the ppc backend.
    
    https://bugs.kde.org/show_bug.cgi?id=430354