| Summary: | epoll_ctl triggers valgrind warnings for uninitialized padding on non-x86_64 64bit arches | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | Mark Wielaard <mark> |
| Component: | general | Assignee: | Julian Seward <jseward> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | Simon.Richter |
| Priority: | NOR | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| URL: | https://bugzilla.redhat.com/show_bug.cgi?id=1844778 | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
| Attachments: | epoll_ctl warns for uninitialized padding on non-amd64 64bit arches | ||
*** Bug 415621 has been marked as a duplicate of this bug. *** (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). Created attachment 130417 [details] epoll_ctl warns for uninitialized padding on non-amd64 64bit arches Patch as proposed in comment #2 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 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 |
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.