|
From: <sv...@va...> - 2012-05-26 23:08:53
|
philippe 2012-05-27 00:08:41 +0100 (Sun, 27 May 2012)
New Revision: 12586
Log:
Fix false positive in sys_clone on amd64 when optional args are not given (e.g. child_tidptr)
rev 10493 fixed bug 117564 in syswrap-x86-linux.c.
This commit fixes the same problem in syswrap-amd64-linux.c.
The problem makes memcheck/tests/linux/stack_switch fails (at least on gcc20)
with unexpected
==802== Syscall param clone(child_tidptr) contains uninitialised byte(s)
The problem originates from always checking 3 optional args PRE_read,
while these should be checked only if the corresponding flags are set.
syswrap-{arm,ppc32,ppc64}-linux.c seems to have the same problem
(but no visible effect) : VKI_CLONE_PARENT_SETTID,VKI_CLONE_CHILD_SETTID
and VKI_CLONE_SETTLS not properly handled in the PRE part.
syswrap-s390x-linux.c seems to have the VKI_CLONE_SETTLS part wrong,
but VKI_CLONE_PARENT_SETTID and VKI_CLONE_CHILD_SETTID correct.
Commiting a fix just for amd64 for now.
We probably better make some common code in syswrap-generic.c
to regroup all similar platforms.
Modified files:
trunk/NEWS
trunk/coregrind/m_syswrap/syswrap-amd64-linux.c
Modified: trunk/NEWS (+1 -0)
===================================================================
--- trunk/NEWS 2012-05-26 00:22:39 -23:00 (rev 12585)
+++ trunk/NEWS 2012-05-27 00:08:41 -23:00 (rev 12586)
@@ -106,6 +106,7 @@
299756 For symmetry, --free-fill must be ignored for MEMPOOL_FREE and FREELIKE client requests
n-i-bz Bypass gcc4.4/4.5 wrong code generation causing out of memory or asserts
n-i-bz Add missing gdbserver xml files for shadow registers for ppc32
+n-i-bz Fix false positive in sys_clone on amd64 when optional args are not given (e.g. child_tidptr)
Release 3.7.0 (5 November 2011)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Modified: trunk/coregrind/m_syswrap/syswrap-amd64-linux.c (+19 -5)
===================================================================
--- trunk/coregrind/m_syswrap/syswrap-amd64-linux.c 2012-05-26 00:22:39 -23:00 (rev 12585)
+++ trunk/coregrind/m_syswrap/syswrap-amd64-linux.c 2012-05-27 00:08:41 -23:00 (rev 12586)
@@ -399,21 +399,35 @@
ULong cloneflags;
PRINT("sys_clone ( %lx, %#lx, %#lx, %#lx, %#lx )",ARG1,ARG2,ARG3,ARG4,ARG5);
- PRE_REG_READ5(int, "clone",
+ PRE_REG_READ2(int, "clone",
unsigned long, flags,
- void *, child_stack,
- int *, parent_tidptr,
- int *, child_tidptr,
- void *, tlsaddr);
+ void *, child_stack);
if (ARG1 & VKI_CLONE_PARENT_SETTID) {
+ if (VG_(tdict).track_pre_reg_read) {
+ PRA3("clone", int *, parent_tidptr);
+ }
PRE_MEM_WRITE("clone(parent_tidptr)", ARG3, sizeof(Int));
if (!VG_(am_is_valid_for_client)(ARG3, sizeof(Int), VKI_PROT_WRITE)) {
SET_STATUS_Failure( VKI_EFAULT );
return;
}
}
+ if (ARG1 & VKI_CLONE_SETTLS) {
+ if (VG_(tdict).track_pre_reg_read) {
+ PRA4("clone", vki_modify_ldt_t *, tlsinfo);
+ }
+ PRE_MEM_READ("clone(tlsinfo)", ARG4, sizeof(vki_modify_ldt_t));
+ if (!VG_(am_is_valid_for_client)(ARG4, sizeof(vki_modify_ldt_t),
+ VKI_PROT_READ)) {
+ SET_STATUS_Failure( VKI_EFAULT );
+ return;
+ }
+ }
if (ARG1 & (VKI_CLONE_CHILD_SETTID | VKI_CLONE_CHILD_CLEARTID)) {
+ if (VG_(tdict).track_pre_reg_read) {
+ PRA5("clone", int *, child_tidptr);
+ }
PRE_MEM_WRITE("clone(child_tidptr)", ARG4, sizeof(Int));
if (!VG_(am_is_valid_for_client)(ARG4, sizeof(Int), VKI_PROT_WRITE)) {
SET_STATUS_Failure( VKI_EFAULT );
|
|
From: Florian K. <br...@ac...> - 2012-05-27 02:32:21
|
On 05/26/2012 07:08 PM, sv...@va... wrote:
> philippe 2012-05-27 00:08:41 +0100 (Sun, 27 May 2012)
>
> New Revision: 12586
>
> Log:
> Fix false positive in sys_clone on amd64 when optional args are not given (e.g. child_tidptr)
>
> rev 10493 fixed bug 117564 in syswrap-x86-linux.c.
> This commit fixes the same problem in syswrap-amd64-linux.c.
> The problem makes memcheck/tests/linux/stack_switch fails (at least on gcc20)
> with unexpected
> ==802== Syscall param clone(child_tidptr) contains uninitialised byte(s)
> The problem originates from always checking 3 optional args PRE_read,
> while these should be checked only if the corresponding flags are set.
>
> syswrap-{arm,ppc32,ppc64}-linux.c seems to have the same problem
> (but no visible effect) : VKI_CLONE_PARENT_SETTID,VKI_CLONE_CHILD_SETTID
> and VKI_CLONE_SETTLS not properly handled in the PRE part.
>
> syswrap-s390x-linux.c seems to have the VKI_CLONE_SETTLS part wrong,
> but VKI_CLONE_PARENT_SETTID and VKI_CLONE_CHILD_SETTID correct.
>
I remember that Christian looked at that testcase failure and r12033
fixed it. I have little wisdom in these syscalls, so I'm copying
Christian. Would be good to get this nailed.
Florian
|
|
From: Philippe W. <phi...@sk...> - 2012-05-27 07:38:38
|
On Sat, 2012-05-26 at 22:32 -0400, Florian Krohm wrote: > > syswrap-s390x-linux.c seems to have the VKI_CLONE_SETTLS part wrong, > > but VKI_CLONE_PARENT_SETTID and VKI_CLONE_CHILD_SETTID correct. > > > > I remember that Christian looked at that testcase failure and r12033 > fixed it. I have little wisdom in these syscalls, so I'm copying > Christian. Would be good to get this nailed. Yes, r12033 introduced the fix for the *SETTID cases. >From what I understand, a similar problem can also happen with SETTLS, which I believe is not fixed in s390x. So, the summary on linux is: x86 and amd64 have *SETTID and SETTLS ok. s390x has *SETTID ok. ppc32/ppc64/arm have none of these ok. Darwin: no idea. Philippe |
|
From: Christian B. <bor...@de...> - 2012-05-28 11:01:46
|
>> I remember that Christian looked at that testcase failure and r12033 >> fixed it. I have little wisdom in these syscalls, so I'm copying >> Christian. Would be good to get this nailed. > Yes, r12033 introduced the fix for the *SETTID cases. >>From what I understand, a similar problem can also happen with SETTLS, > which I believe is not fixed in s390x. > > So, the summary on linux is: > x86 and amd64 have *SETTID and SETTLS ok. > s390x has *SETTID ok. Seems that s390 doesnt look at the TLS value at all for definedness checking. So we dont have a false positive here, instead we might miss a real error. Will fix. Christian |