[systemd-devel] [PATCH] util: fix strict aliasing violations in use of struct inotify_event v3

[Lennart Poettering]

On Mon, 22.12.14 18:54, Shawn Paul Landden (shawn at churchofgit.com) wrote:

> There is alot of cleanup that will have to happen to turn on
> -fstrict-aliasing, but I think our code should be "correct" to the
> rule.

Hmm, this seems incomplete, mount.c and path.c also use the structure?

Also, maybe the FOREACH_INOTIFY_EVENT() should be updated slightly, to
take one of the unions as parameter now...

> 
> v2: move union def to header file
> v3: fix syntax
> ---
>  src/journal/sd-journal.c | 6 +++---
>  src/shared/util.c        | 7 +++----
>  src/shared/util.h        | 6 ++++++
>  src/udev/udevd.c         | 6 +++---
>  4 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/journal/sd-journal.c b/src/journal/sd-journal.c
> index d46dc3c..83be4c9 100644
> --- a/src/journal/sd-journal.c
> +++ b/src/journal/sd-journal.c
> @@ -2188,11 +2188,11 @@ _public_ int sd_journal_process(sd_journal *j) {
>          j->last_process_usec = now(CLOCK_MONOTONIC);
>  
>          for (;;) {
> -                uint8_t buffer[INOTIFY_EVENT_MAX] _alignas_(struct inotify_event);
> +                inotify_event_buffer_t buffer;
>                  struct inotify_event *e;
>                  ssize_t l;
>  
> -                l = read(j->inotify_fd, buffer, sizeof(buffer));
> +                l = read(j->inotify_fd, buffer.raw, sizeof(buffer.raw));
>                  if (l < 0) {
>                          if (errno == EAGAIN || errno == EINTR)
>                                  return got_something ? determine_change(j) : SD_JOURNAL_NOP;
> @@ -2202,7 +2202,7 @@ _public_ int sd_journal_process(sd_journal *j) {
>  
>                  got_something = true;
>  
> -                FOREACH_INOTIFY_EVENT(e, buffer, l)
> +                FOREACH_INOTIFY_EVENT(e, &buffer.ev, l)
>                          process_inotify_event(j, e);
>          }
>  }
> diff --git a/src/shared/util.c b/src/shared/util.c
> index 06b6077..fc83da9 100644
> --- a/src/shared/util.c
> +++ b/src/shared/util.c
> @@ -39,7 +39,6 @@
>  #include <linux/tiocl.h>
>  #include <termios.h>
>  #include <stdarg.h>
> -#include <sys/inotify.h>
>  #include <sys/poll.h>
>  #include <ctype.h>
>  #include <sys/prctl.h>
> @@ -2106,7 +2105,7 @@ int acquire_terminal(
>                  assert(notify >= 0);
>  
>                  for (;;) {
> -                        uint8_t buffer[INOTIFY_EVENT_MAX] _alignas_(struct inotify_event);
> +                        inotify_event_buffer_t buffer;
>                          struct inotify_event *e;
>                          ssize_t l;
>  
> @@ -2129,7 +2128,7 @@ int acquire_terminal(
>                                  }
>                          }
>  
> -                        l = read(notify, buffer, sizeof(buffer));
> +                        l = read(notify, buffer.raw, sizeof(buffer.raw));
>                          if (l < 0) {
>                                  if (errno == EINTR || errno == EAGAIN)
>                                          continue;
> @@ -2138,7 +2137,7 @@ int acquire_terminal(
>                                  goto fail;
>                          }
>  
> -                        FOREACH_INOTIFY_EVENT(e, buffer, l) {
> +                        FOREACH_INOTIFY_EVENT(e, &buffer.ev, l) {
>                                  if (e->wd != wd || !(e->mask & IN_CLOSE)) {
>                                          r = -EIO;
>                                          goto fail;
> diff --git a/src/shared/util.h b/src/shared/util.h
> index 1804b8c..b97d8af 100644
> --- a/src/shared/util.h
> +++ b/src/shared/util.h
> @@ -42,6 +42,7 @@
>  #include <locale.h>
>  #include <mntent.h>
>  #include <sys/socket.h>
> +#include <sys/inotify.h>
>  
>  #if SIZEOF_PID_T == 4
>  #  define PID_FMT "%" PRIu32
> @@ -1051,4 +1052,9 @@ int sethostname_idempotent(const char *s);
>               (uint8_t*) (e) < (uint8_t*) (buffer) + (sz); \
>               (e) = (struct inotify_event*) ((uint8_t*) (e) + sizeof(struct inotify_event) + (e)->len))
>  
> +typedef union {
> +        struct inotify_event ev;
> +        uint8_t raw[INOTIFY_EVENT_MAX];
> +} inotify_event_buffer_t;
> +
>  #define laccess(path, mode) faccessat(AT_FDCWD, (path), (mode), AT_SYMLINK_NOFOLLOW)
> diff --git a/src/udev/udevd.c b/src/udev/udevd.c
> index 8bec03e..bd13025 100644
> --- a/src/udev/udevd.c
> +++ b/src/udev/udevd.c
> @@ -816,11 +816,11 @@ static int synthesize_change(struct udev_device *dev) {
>  }
>  
>  static int handle_inotify(struct udev *udev) {
> -        uint8_t buffer[INOTIFY_EVENT_MAX] _alignas_(struct inotify_event);
> +        inotify_event_buffer_t buffer;
>          struct inotify_event *e;
>          ssize_t l;
>  
> -        l = read(fd_inotify, buffer, sizeof(buffer));
> +        l = read(fd_inotify, buffer.raw, sizeof(buffer.raw));
>          if (l < 0) {
>                  if (errno == EAGAIN || errno == EINTR)
>                          return 0;
> @@ -828,7 +828,7 @@ static int handle_inotify(struct udev *udev) {
>                  return log_error_errno(errno, "Failed to read inotify fd: %m");
>          }
>  
> -        FOREACH_INOTIFY_EVENT(e, buffer, l) {
> +        FOREACH_INOTIFY_EVENT(e, &buffer.ev, l) {
>                  struct udev_device *dev;
>  
>                  dev = udev_watch_lookup(udev, e->wd);
> -- 
> 2.1.0
> 
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Lennart

-- 
Lennart Poettering, Red Hat