| Message ID | 1468958751-16836-1-git-send-email-cascardo@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jul 19, 2016 at 05:05:51PM -0300, Thadeu Lima de Souza Cascardo wrote: > FreeBSD returns a socklen of sockaddr_storage when doing an accept on an unix > STREAM socket. The current code will assume it means a sun_path larger than 0. > > That breaks some tests like the one below which don't expect to find "unix::" on > the logs. > > As a Linux abstract address would not have a more useful name either, it's > better to check that sun_path starts with a non-zero byte and return 0 length in > case it doesn't. > > 402: ovs-ofctl replace-flows with --bundle FAILED (ovs-ofctl.at:2928) > 2016-07-08T12:44:30.068Z|00020|vconn|DBG|unix:: sent (Success): OFPT_HELLO (OF1.6) (xid=0x1): > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> Applied, thanks! If this should be backported to an earlier release branch, let me know and I'll take care of it.
On 19 July 2016 at 20:08, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Jul 19, 2016 at 05:05:51PM -0300, Thadeu Lima de Souza Cascardo wrote: >> FreeBSD returns a socklen of sockaddr_storage when doing an accept on an unix >> STREAM socket. The current code will assume it means a sun_path larger than 0. >> >> That breaks some tests like the one below which don't expect to find "unix::" on >> the logs. >> >> As a Linux abstract address would not have a more useful name either, it's >> better to check that sun_path starts with a non-zero byte and return 0 length in >> case it doesn't. >> >> 402: ovs-ofctl replace-flows with --bundle FAILED (ovs-ofctl.at:2928) >> 2016-07-08T12:44:30.068Z|00020|vconn|DBG|unix:: sent (Success): OFPT_HELLO (OF1.6) (xid=0x1): >> >> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > Applied, thanks! For what it's worth, this introduces a warning on my Linux x86_64 machine with valgrind: ==18725== Conditional jump or move depends on uninitialised value(s) ==18725== at 0x45BF65: get_unix_name_len (socket-util-unix.c:392) ==18725== by 0x45C842: punix_accept (stream-unix.c:113) ==18725== by 0x46897B: pfd_accept (stream-fd.c:263) ==18725== by 0x44CF6F: pstream_accept (stream.c:557) ==18725== by 0x44FE6C: unixctl_server_run (unixctl.c:385) ==18725== by 0x4081AC: main_loop (ovsdb-server.c:182) ==18725== by 0x406432: main (ovsdb-server.c:429)
On Fri, Sep 09, 2016 at 01:47:01PM -0700, Joe Stringer wrote: > On 19 July 2016 at 20:08, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Jul 19, 2016 at 05:05:51PM -0300, Thadeu Lima de Souza Cascardo wrote: > >> FreeBSD returns a socklen of sockaddr_storage when doing an accept on an unix > >> STREAM socket. The current code will assume it means a sun_path larger than 0. > >> > >> That breaks some tests like the one below which don't expect to find "unix::" on > >> the logs. > >> > >> As a Linux abstract address would not have a more useful name either, it's > >> better to check that sun_path starts with a non-zero byte and return 0 length in > >> case it doesn't. > >> > >> 402: ovs-ofctl replace-flows with --bundle FAILED (ovs-ofctl.at:2928) > >> 2016-07-08T12:44:30.068Z|00020|vconn|DBG|unix:: sent (Success): OFPT_HELLO (OF1.6) (xid=0x1): > >> > >> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > > > Applied, thanks! > > For what it's worth, this introduces a warning on my Linux x86_64 > machine with valgrind: > > ==18725== Conditional jump or move depends on uninitialised value(s) > ==18725== at 0x45BF65: get_unix_name_len (socket-util-unix.c:392) > ==18725== by 0x45C842: punix_accept (stream-unix.c:113) > ==18725== by 0x46897B: pfd_accept (stream-fd.c:263) > ==18725== by 0x44CF6F: pstream_accept (stream.c:557) > ==18725== by 0x44FE6C: unixctl_server_run (unixctl.c:385) > ==18725== by 0x4081AC: main_loop (ovsdb-server.c:182) > ==18725== by 0x406432: main (ovsdb-server.c:429) Hum. Is valgrind capable of verifying that sockaddr was written to or not by a successful accept call? I don't know how it works, so not sure if this is a false possible or not. Regards. Cascardo.
diff --git a/lib/socket-util-unix.c b/lib/socket-util-unix.c index 32f966d..5d4b88c 100644 --- a/lib/socket-util-unix.c +++ b/lib/socket-util-unix.c @@ -387,9 +387,10 @@ error: } int -get_unix_name_len(socklen_t sun_len) +get_unix_name_len(const struct sockaddr_un *sun, socklen_t sun_len) { - return (sun_len >= offsetof(struct sockaddr_un, sun_path) + return (sun_len >= offsetof(struct sockaddr_un, sun_path) && + sun->sun_path[0] != 0 ? sun_len - offsetof(struct sockaddr_un, sun_path) : 0); } diff --git a/lib/socket-util.h b/lib/socket-util.h index c3c1224..5bf76a4 100644 --- a/lib/socket-util.h +++ b/lib/socket-util.h @@ -21,6 +21,7 @@ #include <sys/types.h> #include <sys/socket.h> #include <sys/time.h> +#include <sys/un.h> #include <netinet/in.h> #include <stdbool.h> #include "openvswitch/types.h" @@ -84,7 +85,7 @@ int drain_rcvbuf(int fd); int make_unix_socket(int style, bool nonblock, const char *bind_path, const char *connect_path); -int get_unix_name_len(socklen_t sun_len); +int get_unix_name_len(const struct sockaddr_un *sun, socklen_t sun_len); /* Helpers for calling ioctl() on an AF_INET socket. */ struct ifreq; diff --git a/lib/stream-unix.c b/lib/stream-unix.c index cadd180..6424d3e 100644 --- a/lib/stream-unix.c +++ b/lib/stream-unix.c @@ -110,7 +110,7 @@ punix_accept(int fd, const struct sockaddr_storage *ss, size_t ss_len, struct stream **streamp) { const struct sockaddr_un *sun = (const struct sockaddr_un *) ss; - int name_len = get_unix_name_len(ss_len); + int name_len = get_unix_name_len(sun, ss_len); char name[128]; if (name_len > 0) {
FreeBSD returns a socklen of sockaddr_storage when doing an accept on an unix STREAM socket. The current code will assume it means a sun_path larger than 0. That breaks some tests like the one below which don't expect to find "unix::" on the logs. As a Linux abstract address would not have a more useful name either, it's better to check that sun_path starts with a non-zero byte and return 0 length in case it doesn't. 402: ovs-ofctl replace-flows with --bundle FAILED (ovs-ofctl.at:2928) 2016-07-08T12:44:30.068Z|00020|vconn|DBG|unix:: sent (Success): OFPT_HELLO (OF1.6) (xid=0x1): Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> --- lib/socket-util-unix.c | 5 +++-- lib/socket-util.h | 3 ++- lib/stream-unix.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-)