Bug 422623 - epoll_ctl triggers valgrind warnings for uninitialized padding on non-x86_64 64bit arches
Summary: epoll_ctl triggers valgrind warnings for uninitialized padding on non-x86_64 ...
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL: https://bugzilla.redhat.com/show_bug....
Keywords:
: 415621 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-06-08 10:48 UTC by Mark Wielaard
Modified: 2020-08-18 22:05 UTC (History)
1 user (show)

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


Attachments
epoll_ctl warns for uninitialized padding on non-amd64 64bit arches (3.89 KB, text/plain)
2020-07-26 20:47 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-06-08 10:48:53 UTC
We check the event param with:
PRE_MEM_READ( "epoll_ctl(event)", ARG4, sizeof(struct vki_epoll_event) );

But struct vki_epoll_event is defined as:

#ifdef __x86_64__
#define VKI_EPOLL_PACKED __attribute__((packed))
#else
#define VKI_EPOLL_PACKED
#endif

struct vki_epoll_event {
        __vki_u32 events;
        __vki_u64 data;
} VKI_EPOLL_PACKED;

Which mimics the kernel headers and means it isn't packed on non-x86_64 arches and might contain padding on such architectures (as it does on arm64).

This can be solved with something like the following:

diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-l
index f1ecbbf..e94c448 100644
--- a/coregrind/m_syswrap/syswrap-linux.c
+++ b/coregrind/m_syswrap/syswrap-linux.c
@@ -1923,8 +1923,13 @@ PRE(sys_epoll_ctl)
          SARG1, ( ARG2<3 ? epoll_ctl_s[ARG2] : "?" ), SARG3, ARG4);
    PRE_REG_READ4(long, "epoll_ctl",
                  int, epfd, int, op, int, fd, struct vki_epoll_event *, event);
-   if (ARG2 != VKI_EPOLL_CTL_DEL)
-      PRE_MEM_READ( "epoll_ctl(event)", ARG4, sizeof(struct vki_epoll_event) );
+   if (ARG2 != VKI_EPOLL_CTL_DEL) {
+      struct vki_epoll_event *event = (struct vki_epoll_event *) ARG4;
+      PRE_MEM_READ( "epoll_ctl(event.events)", (Addr) &event->events,
+                    sizeof(__vki_u32) );
+      PRE_MEM_READ( "epoll_ctl(event.data)", (Addr) &event->data,
+                    sizeof(__vki_u64) );
+   }
 }
 
 PRE(sys_epoll_wait)

But as pointed out in the Fedora bug report (https://bugzilla.redhat.com/show_bug.cgi?id=1844778) even that might not be completely correct since epoll_data_t is a union of both 32- and 64-bit fields and so might only have seen half of the 64-bits be defined.
Comment 1 Mark Wielaard 2020-07-05 21:45:08 UTC
*** Bug 415621 has been marked as a duplicate of this bug. ***
Comment 2 Mark Wielaard 2020-07-05 22:21:15 UTC
(In reply to Mark Wielaard from comment #0)
> But as pointed out in the Fedora bug report
> (https://bugzilla.redhat.com/show_bug.cgi?id=1844778) even that might not be
> completely correct since epoll_data_t is a union of both 32- and 64-bit
> fields and so might only have seen half of the 64-bits be defined.

This is indeed tricky. The epoll_event data field is user data that the kernel will provide to epoll_[p]wait. Normally one would set data.fd to the fd, otherwise you won't know which file descriptor the event belongs to. But this is not required. Setting just fd would indeed mean only 32bits are initialized (although some example code explicitly does data.u64 = fd).

I don't immediately see how we can track whether or not (and how much of the union) the user sets the data field. So I propose for epoll_ctl we only check the events field is initialized and on epoll_[p]wait we assume both the events and data fields are completely initialized (note that epoll_[p]wait also assumes the padding is initialized, I propose we don't).
Comment 3 Mark Wielaard 2020-07-26 20:47:38 UTC
Created attachment 130417 [details]
epoll_ctl warns for uninitialized padding on non-amd64 64bit arches

Patch as proposed in comment #2
Comment 4 Mark Wielaard 2020-08-01 14:18:18 UTC
commit ecf5ba1197c3833759b40712c401f0bdc768589f
Author: Mark Wielaard <mark@klomp.org>
Date:   Sun Jul 26 22:40:22 2020 +0200

    epoll_ctl warns for uninitialized padding on non-amd64 64bit arches
    
    struct vki_epoll_event is packed on x86_64, but not on other 64bit
    arches. This means that on 64bit arches there can be padding in the
    epoll_event struct. Seperately the data field is only used by user
    space (which might not set the data field if it doesn't need to).
    
    Only check the events field on epoll_ctl. But assume both events
    and data are both written to by epoll_[p]wait (exclude padding).
    
    https://bugs.kde.org/show_bug.cgi?id=422623
Comment 5 Mark Wielaard 2020-08-18 22:05:26 UTC
commit b74f9f23c8758c77367f18368ea95baa858544cb
Author: Mark Wielaard <mark@klomp.org>
Date:   Tue Aug 18 23:58:55 2020 +0200

    Fix epoll_ctl setting of array event and data fields.
    
    Fix for https://bugs.kde.org/show_bug.cgi?id=422623 in commit ecf5ba119
    epoll_ctl warns for uninitialized padding on non-amd64 64bit arches
    contained a bug. A pointer to an array is not a pointer to a pointer to
    an array. Found by a Fedora user:
    https://bugzilla.redhat.com/show_bug.cgi?id=1844778#c10