Bug 359201 - futex syscall "skips" argument 5 if op is FUTEX_WAIT_BITSET
Summary: futex syscall "skips" argument 5 if op is FUTEX_WAIT_BITSET
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: 3.11 SVN
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL: https://bugzilla.redhat.com/show_bug....
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-09 20:35 UTC by Mark Wielaard
Modified: 2016-02-18 11:15 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2016-02-09 20:35:44 UTC
The following program:

#include <chrono>
#include <condition_variable>
#include <mutex>
#include <thread>

using namespace std;

mutex                  m;
timed_mutex            tmx;
condition_variable     cv;
bool                   ready = false;
bool                   finished = false;

bool IsReady()
{
    return ready;
}

bool IsFinished()
{
    return finished;
}

void worker()
{
    // Wait until main thread tells us we can proceed
    unique_lock< mutex > lk( m );
    cv.wait_for( lk, chrono::seconds( 10 ), &IsReady );

    unique_lock< timed_mutex > lock( tmx, chrono::milliseconds( 1 ) );

    finished = true;
    lk.unlock();
    cv.notify_one();
}

int main()
{
    unique_lock< timed_mutex > lock( tmx );

    thread workerThread( &worker );

    // tell worker() to proceed
    {
        unique_lock< mutex > lk( m );
        ready = true;
    }
    cv.notify_one();

    {
        // wait for the worker()
        unique_lock< mutex > lk( m );
        cv.wait_for( lk, chrono::milliseconds( 100 ), &IsFinished );
    }

    workerThread.join();

    return 0;
}

Compiled with g++ 4.8.5 and using glibc-2.17-106 on x86_64 (newer versions don't exhibit the issue) g++ -std=c++11 -pthread -g -O0 -o mutex.bin test.cpp will produce:


==5930== Syscall param futex(dummy) contains uninitialised byte(s)
==5930==    at 0x5663F8C: __lll_timedlock_wait (lowlevellock.S:190)
==5930==    by 0x5660A97: _L_timedlock_67 (pthread_mutex_timedlock.c:236)
==5930==    by 0x566032B: pthread_mutex_timedlock (pthread_mutex_timedlock.c:83)
==5930==    by 0x401108: __gthread_mutex_timedlock(pthread_mutex_t*, timespec const*) (gthr-default.h:768)
==5930==    by 0x4029A7: bool std::timed_mutex::_M_try_lock_until<std::chrono::duration<long, std::ratio<1l, 1000000000l> > >(std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > > const&) (mutex:286)
==5930==    by 0x4023DE: bool std::timed_mutex::_M_try_lock_for<long, std::ratio<1l, 1000l> >(std::chrono::duration<long, std::ratio<1l, 1000l> > const&) (mutex:267)
==5930==    by 0x401FBE: bool std::timed_mutex::try_lock_for<long, std::ratio<1l, 1000l> >(std::chrono::duration<long, std::ratio<1l, 1000l> > const&) (mutex:240)
==5930==    by 0x401AFC: std::unique_lock<std::timed_mutex>::unique_lock<long, std::ratio<1l, 1000l> >(std::timed_mutex&, std::chrono::duration<long, std::ratio<1l, 1000l> > const&) (mutex:470)
==5930==    by 0x40128B: worker() (test.cpp:30)
==5930==    by 0x403504: void std::_Bind_simple<void (*())()>::_M_invoke<>(std::_Index_tuple<>) (functional:1732)
==5930==    by 0x40345E: std::_Bind_simple<void (*())()>::operator()() (functional:1720)
==5930==    by 0x4033F7: std::thread::_Impl<std::_Bind_simple<void (*())()> >::_M_run() (thread:115)

Reproducible: Always




What seems to happen is that argument 5 (uaddr2 aka dummy) really is not used. That is also what the futex manpage says for FUTEX_WAIT_BITSET says.
http://man7.org/linux/man-pages/man2/futex.2.html

But for some reason in this particular case we call futex FUTEXT_WAIT_BITSET with that argument register really containing some uninitialized stuff and get a warning. We really should not check argument 5, but we should check argument 6.

Proposed patch, that fixes the issue for me:

diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c
index f796969..b57436c 100644
--- a/coregrind/m_syswrap/syswrap-linux.c
+++ b/coregrind/m_syswrap/syswrap-linux.c
@@ -1154,13 +1154,16 @@ PRE(sys_futex)
             return;
       }
       if (*(vki_u32 *)ARG1 != ARG3) {
-         PRE_REG_READ5(long, "futex",
+         PRE_REG_READ4(long, "futex",
                        vki_u32 *, futex, int, op, int, val,
-                       struct timespec *, utime, int, dummy);
+                       struct timespec *, utime);
       } else {
-         PRE_REG_READ6(long, "futex",
+        /* Note argument 5 is unused, but argument 6 is used.
+           So we cannot just PRE_REG_READ6. Read argument 6 separately.  */
+         PRE_REG_READ4(long, "futex",
                        vki_u32 *, futex, int, op, int, val,
-                       struct timespec *, utime, int, dummy, int, val3);
+                       struct timespec *, utime);
+         PRA6("futex",int,val3);
       }
       break;
    case VKI_FUTEX_WAKE_BITSET:
Comment 1 Mark Wielaard 2016-02-17 20:53:51 UTC
valgrind svn r15793
Comment 2 Mark Wielaard 2016-02-18 10:44:34 UTC
This caused regressions in the following two tests:
helgrind/tests/cond_timedwait_test       (stderr)
drd/tests/pth_inconsistent_cond_wait     (stderr)
Comment 3 Mark Wielaard 2016-02-18 11:15:43 UTC
valgrind svn r15795

    The original fix in svn r15793 read argument 6 separately by using PRA6
    unconditionally. This is wrong. We need to first check whether a
    track_pre_reg_read callback is registered (only memcheck does).
    The PRE_REG_READX macro already had this check. Just add the same
    before calling PRA6. Thanks to Tom Hughes for noticing