sockets: improve error reporting if UNIX socket path is too long
PATCH v2 1/1 sockets: improve error reporting if UNIX socket path is too long
The 'struct sockaddr_un' only allows 108 bytes for the socket path.
If the user supplies a path, QEMU uses snprintf() to silently truncate it when too long. This is undesirable because the user will then be unable to connect to the path they asked for.
If the user doesn't supply a path, QEMU builds one based on TMPDIR, but if that leads to an overlong path, it mistakenly uses error_setg_errno() with a stale errno value, because snprintf() does not set errno on truncation.
--- util/qemu-sockets.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
util/qemu-sockets.c +17 -6
@@ -845,6 +845,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, {struct sockaddr_un un;int sock, fd; +size_t pathlen; sock =qemu_socket (PF_UNIX ,SOCK_STREAM ,0 );if (sock <0 ) {
@@ -854,15 +855,25 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,memset (&un,0 ,sizeof (un)); un.sun_family =AF_UNIX ; -if (saddr->path &&strlen (saddr->path)) { -snprintf (un.sun_path,sizeof (un.sun_path),"%s" , saddr->path); + pathlen = saddr->path ?strlen (saddr->path) :0 ; +if (pathlen !=0 ) { +if (pathlen >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , saddr->path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path)); + goto err; + } +strncpy (un.sun_path, saddr->path,sizeof (un.sun_path)); }else { +const char *template ="qemu-socket-XXXXXX" ;const char *tmpdir =getenv ("TMPDIR" ); tmpdir = tmpdir ? tmpdir :"/tmp" ; -if (snprintf (un.sun_path,sizeof (un.sun_path),"%s/qemu-socket-XXXXXX" , - tmpdir) >=sizeof (un.sun_path)) { -error_setg_errno (errp, errno, -"TMPDIR environment variable (%s) too large" , tmpdir); +if (snprintf (un.sun_path,sizeof (un.sun_path),"%s/%s" , + tmpdir, template) >=sizeof (un.sun_path)) { +error_setg (errp, +"TMPDIR environment variable (%s) too large" , tmpdir); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path) -strlen (template) -1 ); goto err; }
Signature · 1 line
2.9.3
Quoted 18 lines · Daniel P. Berrange
The 'struct
sockaddr_un' only allows 108 bytes for the socket path.If the user supplies a path, QEMU uses
snprintf()to silently truncate it when too long. This is undesirable because the user will then be unable to connect to the path they asked for.If the user doesn't supply a path, QEMU builds one based on TMPDIR, but if that leads to an overlong path, it mistakenly uses
error_setg_errno()with a stale errno value, becausesnprintf()does not set errno on truncation.Signed-off-by Daniel P. Berrange <[email protected]>--- util/qemu-sockets.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 866
Quoted 9 lines
util/qemu-sockets.c +9
-0+ pathlen = saddr->path ?strlen (saddr->path) :0 ; +if (pathlen !=0 ) { +if (pathlen >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , saddr->path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path)); + goto err; + } +strncpy (un.sun_path, saddr->path,sizeof (un.sun_path));
Does NOT nul-terminate if pathlen is exactly 108 - but it looks like Linux is just fine with a Unix socket name that long.
Quoted 4 lines
} else {util/qemu-sockets.c +1
-0+const char *template ="qemu-socket-XXXXXX" ;const char *tmpdir =getenv ("TMPDIR" ); tmpdir = tmpdir ? tmpdir :"/tmp" ;
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 876
Quoted 10 lines
util/qemu-sockets.c +6
-4-if (snprintf (un.sun_path,sizeof (un.sun_path),"%s/qemu-socket-XXXXXX" , - tmpdir) >=sizeof (un.sun_path)) { -error_setg_errno (errp, errno, -"TMPDIR environment variable (%s) too large" , tmpdir); +if (snprintf (un.sun_path,sizeof (un.sun_path),"%s/%s" , + tmpdir, template) >=sizeof (un.sun_path)) { +error_setg (errp, +"TMPDIR environment variable (%s) too large" , tmpdir); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path) -strlen (template) -1 );
Meanwhile, this branch requires nul-termination (that is, a maximum generated name of 107), but that's because we then pass un.sun_path to mkstemp() and g_strdup(), both of which require nul-termination. It's a bit odd to see the asymmetry between a user-specified name going all the way to 108 while generated cannot, but not the end of the world.
You do, however, have one more problem to fix:
if (unlink(un.sun_path) < 0 && errno != ENOENT) {That now needs to be 'if (unlink(saddr->path) ...', because un.sun_path is no longer guaranteed to be NUL-terminated (although saddr->path is). Likewise for the two error_setg_errno() calls after that point.
With that fixed,
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org