| Summary: | Missing memory check for futex() uaddr arg for FUTEX_WAKE and FUTEX_WAKE_BITSET, check only 4 args for FUTEX_WAKE_BITSET, and 2 args for FUTEX_TRYLOCK_PI | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | Diane M <diane.meirowitz> |
| Component: | memcheck | Assignee: | Ivo Raisr <ivosh> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | diane.meirowitz, ivosh, jseward |
| Priority: | NOR | ||
| Version: | 3.13 SVN | ||
| Target Milestone: | --- | ||
| Platform: | Compiled Sources | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
| Attachments: |
patch
updated patch |
||
In addition to the previously-described problems:
- FUTEX_WAKE_BITSET should not do scalar read check on all 6 arguments;
it should check uaddr, op, val, and val3 only.
- FUTEX_TRYLOCK_PI should check only uaddr and op
Here is a modified patch:
diff -r 231368959406 coregrind/m_syswrap/syswrap-linux.c
--- a/coregrind/m_syswrap/syswrap-linux.c Tue Mar 14 09:47:29 2017 -0700
+++ b/coregrind/m_syswrap/syswrap-linux.c Mon Mar 20 08:26:03 2017 -0700
@@ -1633,9 +1633,11 @@
}
break;
case VKI_FUTEX_WAKE_BITSET:
- PRE_REG_READ6(long, "futex",
- vki_u32 *, futex, int, op, int, val,
- int, dummy, int, dummy2, int, val3);
+ PRE_REG_READ3(long, "futex",
+ vki_u32 *, futex, int, op, int, val);
+ if (VG_(tdict).track_pre_reg_read) {
+ PRA6("futex",int,val3);
+ }
break;
case VKI_FUTEX_WAIT:
case VKI_FUTEX_LOCK_PI:
@@ -1645,11 +1647,11 @@
break;
case VKI_FUTEX_WAKE:
case VKI_FUTEX_FD:
- case VKI_FUTEX_TRYLOCK_PI:
PRE_REG_READ3(long, "futex",
vki_u32 *, futex, int, op, int, val);
break;
case VKI_FUTEX_UNLOCK_PI:
+ case VKI_FUTEX_TRYLOCK_PI:
default:
PRE_REG_READ2(long, "futex", vki_u32 *, futex, int, op);
break;
@@ -1678,14 +1680,11 @@
case VKI_FUTEX_FD:
case VKI_FUTEX_TRYLOCK_PI:
case VKI_FUTEX_UNLOCK_PI:
+ case VKI_FUTEX_WAKE:
+ case VKI_FUTEX_WAKE_BITSET:
PRE_MEM_READ( "futex(futex)", ARG1, sizeof(Int) );
break;
- case VKI_FUTEX_WAKE:
- case VKI_FUTEX_WAKE_BITSET:
- /* no additional pointers */
- break;
-
default:
SET_STATUS_Failure( VKI_ENOSYS ); // some futex function we don't understand
break;
Here is the final updated patch.
Problems:
1. Memcheck does not do a memory check on the uaddr argument for futex for FUTEX_WAKE and FUTEX_WAKE_BITSET cases.
2. FUTEX_WAKE_BITSET should not do scalar read check on all 6 arguments;
it should check uaddr, op, val, and val3 only.
3. FUTEX_TRYLOCK_PI should check only uaddr and op
diff -r 231368959406 coregrind/m_syswrap/syswrap-linux.c
--- a/coregrind/m_syswrap/syswrap-linux.c Tue Mar 14 09:47:29 2017 -0700
+++ b/coregrind/m_syswrap/syswrap-linux.c Tue Mar 21 08:11:21 2017 -0700
@@ -1633,9 +1633,11 @@
}
break;
case VKI_FUTEX_WAKE_BITSET:
- PRE_REG_READ6(long, "futex",
- vki_u32 *, futex, int, op, int, val,
- int, dummy, int, dummy2, int, val3);
+ PRE_REG_READ3(long, "futex",
+ vki_u32 *, futex, int, op, int, val);
+ if (VG_(tdict).track_pre_reg_read) {
+ PRA6("futex", int, val3);
+ }
break;
case VKI_FUTEX_WAIT:
case VKI_FUTEX_LOCK_PI:
@@ -1645,11 +1647,11 @@
break;
case VKI_FUTEX_WAKE:
case VKI_FUTEX_FD:
- case VKI_FUTEX_TRYLOCK_PI:
PRE_REG_READ3(long, "futex",
vki_u32 *, futex, int, op, int, val);
break;
case VKI_FUTEX_UNLOCK_PI:
+ case VKI_FUTEX_TRYLOCK_PI:
default:
PRE_REG_READ2(long, "futex", vki_u32 *, futex, int, op);
break;
@@ -1678,14 +1680,11 @@
case VKI_FUTEX_FD:
case VKI_FUTEX_TRYLOCK_PI:
case VKI_FUTEX_UNLOCK_PI:
+ case VKI_FUTEX_WAKE:
+ case VKI_FUTEX_WAKE_BITSET:
PRE_MEM_READ( "futex(futex)", ARG1, sizeof(Int) );
break;
- case VKI_FUTEX_WAKE:
- case VKI_FUTEX_WAKE_BITSET:
- /* no additional pointers */
- break;
-
default:
SET_STATUS_Failure( VKI_ENOSYS ); // some futex function we don't understand
break;
diff -r 231368959406 memcheck/tests/arm64-linux/scalar.c
--- a/memcheck/tests/arm64-linux/scalar.c Tue Mar 14 09:47:29 2017 -0700
+++ b/memcheck/tests/arm64-linux/scalar.c Tue Mar 21 08:11:21 2017 -0700
@@ -1067,9 +1067,8 @@
#ifndef FUTEX_WAIT
#define FUTEX_WAIT 0
#endif
- // XXX: again, glibc not doing 6th arg means we have only 5s errors
- GO(__NR_futex, "5s 2m");
- SY(__NR_futex, x0+FUTEX_WAIT, x0, x0, x0+1, x0, x0); FAIL;
+ GO(__NR_futex, "4s 2m");
+ SY(__NR_futex, x0+FUTEX_WAIT, x0, x0, x0+1); FAIL;
// __NR_sched_setaffinity 241
GO(__NR_sched_setaffinity, "3s 1m");
diff -r 231368959406 memcheck/tests/arm64-linux/scalar.stderr.exp
--- a/memcheck/tests/arm64-linux/scalar.stderr.exp Tue Mar 14 09:47:29 2017 -0700
+++ b/memcheck/tests/arm64-linux/scalar.stderr.exp Tue Mar 21 08:11:21 2017 -0700
@@ -1958,7 +1958,7 @@
130: __NR_tkill n/a
-----------------------------------------------------
-----------------------------------------------------
- 98: __NR_futex 5s 2m
+ 98: __NR_futex 4s 2m
-----------------------------------------------------
Syscall param futex(futex) contains uninitialised byte(s)
...
diff -r 231368959406 memcheck/tests/darwin/scalar.c
--- a/memcheck/tests/darwin/scalar.c Tue Mar 14 09:47:29 2017 -0700
+++ b/memcheck/tests/darwin/scalar.c Tue Mar 21 08:11:21 2017 -0700
@@ -1653,9 +1653,8 @@
#ifndef FUTEX_WAIT
#define FUTEX_WAIT 0
#endif
- // XXX: again, glibc not doing 6th arg means we have only 5s errors
- GO(__NR_futex, "5s 2m");
- SY(__NR_futex, x0+FUTEX_WAIT, x0, x0, x0+1, x0, x0); FAIL;
+ GO(__NR_futex, "4s 2m");
+ SY(__NR_futex, x0+FUTEX_WAIT, x0, x0, x0+1); FAIL;
// __NR_sched_setaffinity 241
GO(__NR_sched_setaffinity, "3s 1m");
diff -r 231368959406 memcheck/tests/x86-linux/scalar.c
--- a/memcheck/tests/x86-linux/scalar.c Tue Mar 14 09:47:29 2017 -0700
+++ b/memcheck/tests/x86-linux/scalar.c Tue Mar 21 08:11:21 2017 -0700
@@ -1067,9 +1067,8 @@
#ifndef FUTEX_WAIT
#define FUTEX_WAIT 0
#endif
- // XXX: again, glibc not doing 6th arg means we have only 5s errors
- GO(__NR_futex, "5s 2m");
- SY(__NR_futex, x0+FUTEX_WAIT, x0, x0, x0+1, x0, x0); FAIL;
+ GO(__NR_futex, "4s 2m");
+ SY(__NR_futex, x0+FUTEX_WAIT, x0, x0, x0+1); FAIL;
// __NR_sched_setaffinity 241
GO(__NR_sched_setaffinity, "3s 1m");
diff -r 231368959406 memcheck/tests/x86-linux/scalar.stderr.exp
--- a/memcheck/tests/x86-linux/scalar.stderr.exp Tue Mar 14 09:47:29 2017 -0700
+++ b/memcheck/tests/x86-linux/scalar.stderr.exp Tue Mar 21 08:11:21 2017 -0700
@@ -3300,7 +3300,7 @@
Address 0x........ is not stack'd, malloc'd or (recently) free'd
-----------------------------------------------------
-240: __NR_futex 5s 2m
+240: __NR_futex 4s 2m
-----------------------------------------------------
Syscall param futex(futex) contains uninitialised byte(s)
...
Created attachment 104671 [details]
patch
Diane, your patch does not apply to Valgrind SVN trunk. 1. It is malformed. Line 48 seems to be split somehow. 2. After correcting item 1 manually, 2 hunks in coregrind/m_syswrap/syswrap-linux.c fail to apply. Please submit a new patch which applies cleanly to Valgrind SVN trunk. As background, how did you come across this problem? Did you see some
false errors in some test cases, or something like that?
Looks ok to me, except
+ if (VG_(tdict).track_pre_reg_read) {
+ PRA6("futex",int,val3);
+ }
I think the PRA6 call shouldn't be guarded by VG_(tdict).track_pre_reg_read,
since -- as far as I can see -- PRA6 tests that anyway. I see that that's a
pre-existing problem in this syswrap-linux.c, and IMO all of the explicit
checks of VG_(tdict).track_pre_reg_read in this file are redundant.
Please attach patches in files (the "paste text as attachment" option is
convenient, sometimes). This avoids corruption problems. Also, scraping
patches off the web page when it comes time to land them is difficult and
error-prone.
Julian, I encountered the problem while implementing and testing futex() for sparc linux.
+ if (VG_(tdict).track_pre_reg_read) {
+ PRA6("futex",int,val3);
+ }
I believe the if is necessary, because it is checking whether the function pointer is defined, whereas PRA6 eventually invokes this code which actually calls the function pointed to.
#define PRRAn_LE(n,s,t,a) \
do { \
Int here = layout->o_arg##n; \
vg_assert(sizeof(t) <= sizeof(UWord)); \
vg_assert(here >= 0); \
VG_(tdict).track_pre_reg_read( \
Vg_CoreSysCall, tid, s"("#a")", \
here, sizeof(t) \
); \
} while (0)
(In reply to Diane M from comment #6) > I believe the if is necessary, because it is checking whether the function > pointer is defined, [..] Oh, you're right. I missed that. r+ then; can you attach a patch that builds and tests OK agains the trunk? Created attachment 104704 [details]
updated patch
I hope this patch works better.
Committed in SVN r16285. |
Memcheck does not do a memory check on the uaddr argument for futex for FUTEX_WAKE and FUTEX_WAKE_BITSET cases. According to the Linux sources, the uaddr argument is dereferenced in all cases. Here is a patch to fix this: diff -r 231368959406 coregrind/m_syswrap/syswrap-linux.c --- a/coregrind/m_syswrap/syswrap-linux.c Tue Mar 14 09:47:29 2017 -0700 +++ b/coregrind/m_syswrap/syswrap-linux.c Wed Mar 15 12:18:29 2017 -0700 @@ -1678,14 +1678,11 @@ case VKI_FUTEX_FD: case VKI_FUTEX_TRYLOCK_PI: case VKI_FUTEX_UNLOCK_PI: + case VKI_FUTEX_WAKE: + case VKI_FUTEX_WAKE_BITSET: PRE_MEM_READ( "futex(futex)", ARG1, sizeof(Int) ); break; - case VKI_FUTEX_WAKE: - case VKI_FUTEX_WAKE_BITSET: - /* no additional pointers */ - break; - I am testing this now. It is likely that the only test cases the patch will affect is memcheck/tests/*/scalar.c.