3

PVS-Studio, a static analyzer, reports that in nh = (struct nlmsghdr *) buf,

The pointer 'buf' is cast to a more strictly aligned pointer type.

I think that the warning is correct. Is this a serious problem? The code needs to be portable on various architectures. How can I fix this?

I know of some ways:

  • allocate the buffer on the heap;
  • use the stdalign features, but they’re not C99,
  • the cmsghdr api uses an union to deal with the alignment, but I’m not sure if netlink does that.

Is there another option?

The code below is taken from the netlink manpage.

int len;
char buf[8192];     /* 8192 to avoid message truncation on
                       platforms with page size > 4096 */
struct iovec iov = { buf, sizeof(buf) };
struct sockaddr_nl sa;
struct msghdr msg;
struct nlmsghdr *nh;

msg = { &sa, sizeof(sa), &iov, 1, NULL, 0, 0 };
len = recvmsg(fd, &msg, 0);

for (nh = (struct nlmsghdr *) buf; NLMSG_OK (nh, len);
     nh = NLMSG_NEXT (nh, len)) {
     /* The end of multipart message */
     if (nh->nlmsg_type == NLMSG_DONE)
     return;

     if (nh->nlmsg_type == NLMSG_ERROR)
     /* Do some error handling */
     ...

     /* Continue with parsing payload */
     ...
}

Thanks.

Antonin Décimo
  • 499
  • 3
  • 17
  • 1
    The code invokes undefined behavior because it violates both [strict aliasing](https://stackoverflow.com/questions/98650/what-is-the-strict-aliasing-rule) and [**6.3.2.3 Pointers**, paragraph 7](https://port70.net/~nsz/c/c11/n1570.html#6.3.2.3p7): "A pointer to an object type may be converted to a pointer to a different object type. If the resulting pointer is not correctly aligned for the referenced type, **the behavior is undefined.**" It's not possible to safely alias an actual array of `char` as something else. Just Google "`SIGBUS` arm" or "`SIGBUS` sparc". – Andrew Henle Sep 01 '19 at 11:52
  • Thanks for pointing that out. Should this be reported to the netlink project? – Antonin Décimo Sep 01 '19 at 12:38

1 Answers1

2

Isn't better to take a pointer of an allocated struct nlmsghdr instead of a char buf[8192]?

for example:

int len;
struct nlmsghdr buf;
struct iovec iov = { &buf, sizeof(buf) };
struct sockaddr_nl sa;
struct msghdr msg;
struct nlmsghdr *nh;

msg = { &sa, sizeof(sa), &iov, 1, NULL, 0, 0 };
len = recvmsg(fd, &msg, 0);

for (nh = &buf; NLMSG_OK (nh, len);
    nh = NLMSG_NEXT (nh, len)) {
    /* The end of multipart message */
    if (nh->nlmsg_type == NLMSG_DONE)
    return;

    if (nh->nlmsg_type == NLMSG_ERROR)
    /* Do some error handling */
    ...

    /* Continue with parsing payload */
    ...
}
Memos Electron
  • 660
  • 5
  • 26
  • *`sizeof(buf)` gives the size of the pointer not of the array.* Not in the posted code as the full definition of the array is available. – Andrew Henle Sep 01 '19 at 11:41
  • I’m not sure, but I think that in this example only one nlmsghdr structure will be received, whereas with the `char` buffer, multiple messages can be appended. – Antonin Décimo Sep 01 '19 at 12:41
  • This is not the only thing that it might be wrong. Also the length of the message in `NLMSG_OK (nh, len)` for example I think you need the `nh->nlmsg_len` instead. I don't have something ready for testing right now. Although if this it is only for 1 msg then just allocate an array of `struct nlmsghdr` like `struct nlmsghdr buf[256]` – Memos Electron Sep 01 '19 at 13:02
  • No, the prototype is `int NLMSG_OK(struct nlmsghdr *nlh, int len);`. I’m going with `struct nlmsghdr buf[8192/sizeof(struct nlmsghdr)];`, and `sizeof(buf)` returns the size in bytes of `buf`, as expected. – Antonin Décimo Sep 01 '19 at 17:05
  • The patch using my solution was accepted; see [ae10667](https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=ae10667d4891a592388f8dc63e1b01fed6a1b4bf). – Antonin Décimo Jan 07 '20 at 12:47