Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Initialize struct epoll_event to all zeros #14353

Closed
wants to merge 1 commit into from
Closed

Initialize struct epoll_event to all zeros #14353

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/libsystemd/sd-event/sd-event.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ static int source_io_register(
int enabled,
uint32_t events) {

struct epoll_event ev;
struct epoll_event ev = {0};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, why would this be any different then literally the same initialization a few lines down?

Also, epoll_ctl() doesn't really care about the padding. Isn't this something to fix in valgrind?

Copy link
Member

@poettering poettering Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, we generally use the GNU C extension that allows us to write = {} rather than = {0}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valgrind is actually right, because in the epoll_ctl syscall, it uses copy_from_user to copy the whole struct into kernel space, and thus the uninitialized bytes are leaked into kernel.

    copy_from_user(&epds, event, sizeof(struct epoll_event)))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that's pretty much OK, no? its pading, so noone should care. And the kernel is more privileged anyway, so you can't really say "leak into". If we'd leak the padding into less privileged stuff this would be a problem, but the other direction?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, we generally use the GNU C extension that allows us to write = {} rather than = {0}

OK, I could update the code to use = {}

but that's pretty much OK, no? its pading, so noone should care. And the kernel is more privileged anyway, so you can't really say "leak into". If we'd leak the padding into less privileged stuff this would be a problem, but the other direction?

I agree it's not really a bug, especially if GCC is doing right we do not need this change after all.

But isn't it better to fix such uninitialized bytes when valgrind helps to identify the issue? (if we accept that GCC is not going to fix it soon, the bug was opened in 2016)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would this be any different then literally the same initialization a few lines down?

I think that this is the biggest issue. Because of some accident of implementation, one initialization has a different effect than the other one. We shouldn't base our work-around something so flimsy.

I'd rather see the declaration moved down to the point of initialization and the cast removed:

struct epoll_event ev = {
         .events = events | (enabled == SD_EVENT_ONESHOT ? EPOLLONESHOT : 0),
          .data.ptr = s,
};

Would this help with your issue?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, the problem is actually in GCC with bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77992

It does not matter if we write

struct epoll_event ev;
ev = (struct epoll_event) {...};

or

struct epoll_event ev = {...};

In both cases, GCC generates uninitialized data if the struct has padding bytes.
Example on coliru: http://coliru.stacked-crooked.com/a/a9a907bda006d9cd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but shouldn't valgrind be fixed to not complain about non-initialized padding to be passed to kernel apis where it's known-safe?

I mean, i can see why complaining about that in write() makes sense, but i fail to see what could be wrong here with epoll. valrgind is just generating incorrect warnings.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's valid code and it should not cause anything wrong at runtime.
But I also appreciate Valgrind telling that there are non-initialized bytes that are accessed, which potentially may cause issues.

So I think manually initializing the padding to 0 is an acceptable workaround.

int r;

assert(s);
Expand Down Expand Up @@ -436,7 +436,7 @@ static int source_child_pidfd_register(sd_event_source *s, int enabled) {
assert(enabled != SD_EVENT_OFF);

if (EVENT_SOURCE_WATCH_PIDFD(s)) {
struct epoll_event ev;
struct epoll_event ev = {0};

ev = (struct epoll_event) {
.events = EPOLLIN | (enabled == SD_EVENT_ONESHOT ? EPOLLONESHOT : 0),
Expand Down Expand Up @@ -544,7 +544,7 @@ static int event_make_signal_data(
int sig,
struct signal_data **ret) {

struct epoll_event ev;
struct epoll_event ev = {0};
struct signal_data *d;
bool added = false;
sigset_t ss_copy;
Expand Down Expand Up @@ -1039,7 +1039,7 @@ static int event_setup_timer_fd(
struct clock_data *d,
clockid_t clock) {

struct epoll_event ev;
struct epoll_event ev = {0};
int r, fd;

assert(e);
Expand Down Expand Up @@ -1549,7 +1549,7 @@ static int event_make_inotify_data(

_cleanup_close_ int fd = -1;
struct inotify_data *d;
struct epoll_event ev;
struct epoll_event ev = {0};
int r;

assert(e);
Expand Down Expand Up @@ -3493,7 +3493,7 @@ _public_ int sd_event_wait(sd_event *e, uint64_t timeout) {
}

ev_queue_max = MAX(e->n_sources, 1u);
ev_queue = newa(struct epoll_event, ev_queue_max);
ev_queue = newa0(struct epoll_event, ev_queue_max);

/* If we still have inotify data buffered, then query the other fds, but don't wait on it */
if (e->inotify_data_buffered)
Expand Down Expand Up @@ -3841,7 +3841,7 @@ _public_ int sd_event_set_watchdog(sd_event *e, int b) {
return e->watchdog;

if (b) {
struct epoll_event ev;
struct epoll_event ev = {0};

r = sd_watchdog_enabled(false, &e->watchdog_period);
if (r <= 0)
Expand Down