* [Qemu-devel] [PULL 14/22] nbd/client.c: use errp instead of LOG
2017-05-26 14:38 [Qemu-devel] [PULL v3 00/22] Misc patches for 2017-05-19 Paolo Bonzini
@ 2017-05-26 14:38 ` Paolo Bonzini
2017-05-26 14:38 ` [Qemu-devel] [PULL 21/22] i386: fix read/write cr with icount option Paolo Bonzini
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2017-05-26 14:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Move to modern errp scheme from just LOGging errors.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20170526110913.89098-1-vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd-client.c | 7 ++++++-
include/block/nbd.h | 5 +++--
nbd/client.c | 30 +++++++++++++++++-------------
qemu-nbd.c | 3 ++-
tests/qemu-iotests/083.out | 2 ++
5 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 538d95e031..09d955bc4d 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -28,6 +28,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "nbd-client.h"
#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))
@@ -70,10 +71,14 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
NBDClientSession *s = opaque;
uint64_t i;
int ret;
+ Error *local_err = NULL;
for (;;) {
assert(s->reply.handle == 0);
- ret = nbd_receive_reply(s->ioc, &s->reply);
+ ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
if (ret <= 0) {
break;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9d385ea564..416257abca 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -133,9 +133,10 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
QCryptoTLSCreds *tlscreds, const char *hostname,
QIOChannel **outioc,
off_t *size, Error **errp);
-int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size);
+int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
+ Error **errp);
ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request);
-ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply);
+ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
int nbd_client(int fd);
int nbd_disconnect(int fd);
diff --git a/nbd/client.c b/nbd/client.c
index f102375504..595d99ed30 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -627,11 +627,13 @@ fail:
}
#ifdef __linux__
-int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
+ Error **errp)
{
unsigned long sectors = size / BDRV_SECTOR_SIZE;
if (size / BDRV_SECTOR_SIZE != sectors) {
- LOG("Export size %lld too large for 32-bit kernel", (long long) size);
+ error_setg(errp, "Export size %lld too large for 32-bit kernel",
+ (long long) size);
return -E2BIG;
}
@@ -639,7 +641,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_SOCK, (unsigned long) sioc->fd) < 0) {
int serrno = errno;
- LOG("Failed to set NBD socket");
+ error_setg(errp, "Failed to set NBD socket");
return -serrno;
}
@@ -647,7 +649,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_BLKSIZE, (unsigned long)BDRV_SECTOR_SIZE) < 0) {
int serrno = errno;
- LOG("Failed setting NBD block size");
+ error_setg(errp, "Failed setting NBD block size");
return -serrno;
}
@@ -659,7 +661,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
int serrno = errno;
- LOG("Failed setting size (in blocks)");
+ error_setg(errp, "Failed setting size (in blocks)");
return -serrno;
}
@@ -670,12 +672,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
if (ioctl(fd, BLKROSET, (unsigned long) &read_only) < 0) {
int serrno = errno;
- LOG("Failed setting read-only attribute");
+ error_setg(errp, "Failed setting read-only attribute");
return -serrno;
}
} else {
int serrno = errno;
- LOG("Failed setting flags");
+ error_setg(errp, "Failed setting flags");
return -serrno;
}
}
@@ -723,8 +725,10 @@ int nbd_disconnect(int fd)
}
#else
-int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size,
+ Error **errp)
{
+ error_setg(errp, "nbd_init is only supported on Linux");
return -ENOTSUP;
}
@@ -758,19 +762,19 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
return write_sync(ioc, buf, sizeof(buf), NULL);
}
-ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
+ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
{
uint8_t buf[NBD_REPLY_SIZE];
uint32_t magic;
ssize_t ret;
- ret = read_sync_eof(ioc, buf, sizeof(buf), NULL);
+ ret = read_sync_eof(ioc, buf, sizeof(buf), errp);
if (ret <= 0) {
return ret;
}
if (ret != sizeof(buf)) {
- LOG("read failed");
+ error_setg(errp, "read failed");
return -EINVAL;
}
@@ -788,7 +792,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
if (reply->error == ESHUTDOWN) {
/* This works even on mingw which lacks a native ESHUTDOWN */
- LOG("server shutting down");
+ error_setg(errp, "server shutting down");
return -EINVAL;
}
TRACE("Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32
@@ -796,7 +800,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
magic, reply->error, reply->handle);
if (magic != NBD_REPLY_MAGIC) {
- LOG("invalid magic (got 0x%" PRIx32 ")", magic);
+ error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
return -EINVAL;
}
return sizeof(buf);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b7ab86bfa7..f60842fd86 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -288,8 +288,9 @@ static void *nbd_client_thread(void *arg)
goto out_socket;
}
- ret = nbd_init(fd, sioc, nbdflags, size);
+ ret = nbd_init(fd, sioc, nbdflags, size, &local_error);
if (ret < 0) {
+ error_report_err(local_error);
goto out_fd;
}
diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index 0c13888ba1..a24c6bfece 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -69,10 +69,12 @@ read failed: Input/output error
=== Check disconnect 4 reply ===
+read failed
read failed: Input/output error
=== Check disconnect 8 reply ===
+read failed
read failed: Input/output error
=== Check disconnect before data ===
--
2.13.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] [PULL 22/22] sockets: improve error reporting if UNIX socket path is too long
2017-05-26 14:38 [Qemu-devel] [PULL v3 00/22] Misc patches for 2017-05-19 Paolo Bonzini
2017-05-26 14:38 ` [Qemu-devel] [PULL 14/22] nbd/client.c: use errp instead of LOG Paolo Bonzini
2017-05-26 14:38 ` [Qemu-devel] [PULL 21/22] i386: fix read/write cr with icount option Paolo Bonzini
@ 2017-05-26 14:38 ` Paolo Bonzini
2017-05-30 9:22 ` [Qemu-devel] [PULL v3 00/22] Misc patches for 2017-05-19 Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2017-05-26 14:38 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
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.
In solving this the code needed some refactoring to ensure we
don't pass 'un.sun_path' directly to any APIs which expect
NUL-terminated strings, because the path is not required to
be terminated.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <20170525155300.22743-1-berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
util/qemu-sockets.c | 68 ++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 46 insertions(+), 22 deletions(-)
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d8183f79d7..dfaf4e1d82 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -845,6 +845,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
{
struct sockaddr_un un;
int sock, fd;
+ char *pathbuf = NULL;
+ const char *path;
sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
if (sock < 0) {
@@ -852,20 +854,22 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
return -1;
}
- 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);
+ if (saddr->path && saddr->path[0]) {
+ path = saddr->path;
} else {
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);
- goto err;
- }
+ path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir);
+ }
+ if (strlen(path) > sizeof(un.sun_path)) {
+ error_setg(errp, "UNIX socket path '%s' is too long", path);
+ error_append_hint(errp, "Path must be less than %zu bytes\n",
+ sizeof(un.sun_path));
+ goto err;
+ }
+
+ if (pathbuf != NULL) {
/*
* This dummy fd usage silences the mktemp() unsecure warning.
* Using mkstemp() doesn't make things more secure here
@@ -873,24 +877,25 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
* to unlink first and thus re-open the race window. The
* worst case possible is bind() failing, i.e. a DoS attack.
*/
- fd = mkstemp(un.sun_path);
+ fd = mkstemp(pathbuf);
if (fd < 0) {
error_setg_errno(errp, errno,
- "Failed to make a temporary socket name in %s", tmpdir);
+ "Failed to make a temporary socket %s", pathbuf);
goto err;
}
close(fd);
- if (update_addr) {
- g_free(saddr->path);
- saddr->path = g_strdup(un.sun_path);
- }
}
- if (unlink(un.sun_path) < 0 && errno != ENOENT) {
+ if (unlink(path) < 0 && errno != ENOENT) {
error_setg_errno(errp, errno,
- "Failed to unlink socket %s", un.sun_path);
+ "Failed to unlink socket %s", path);
goto err;
}
+
+ memset(&un, 0, sizeof(un));
+ un.sun_family = AF_UNIX;
+ strncpy(un.sun_path, path, sizeof(un.sun_path));
+
if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
error_setg_errno(errp, errno, "Failed to bind socket to %s", un.sun_path);
goto err;
@@ -900,9 +905,16 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
goto err;
}
+ if (update_addr && pathbuf) {
+ g_free(saddr->path);
+ saddr->path = pathbuf;
+ } else {
+ g_free(pathbuf);
+ }
return sock;
err:
+ g_free(pathbuf);
closesocket(sock);
return -1;
}
@@ -932,9 +944,16 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,
qemu_set_nonblock(sock);
}
+ if (strlen(saddr->path) > 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;
+ }
+
memset(&un, 0, sizeof(un));
un.sun_family = AF_UNIX;
- snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path);
+ strncpy(un.sun_path, saddr->path, sizeof(un.sun_path));
/* connect to peer */
do {
@@ -956,13 +975,18 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,
}
if (rc < 0) {
- error_setg_errno(errp, -rc, "Failed to connect socket");
- close(sock);
- sock = -1;
+ error_setg_errno(errp, -rc, "Failed to connect socket %s",
+ saddr->path);
+ goto err;
}
g_free(connect_state);
return sock;
+
+ err:
+ close(sock);
+ g_free(connect_state);
+ return -1;
}
#else
--
2.13.0
^ permalink raw reply related [flat|nested] 5+ messages in thread