| Message ID | 20190115145256.9593-2-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | chardev: refactoring & many bugfixes related tcp_chr_wait_connected | expand |
Hi On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > The TLS creds option is not valid with certain address types. The user > config was only checked for errors when parsing legacy QemuOpts, thus > the user could pass unsupported values via QMP. > > Pull all code for validating options out into a new method > qmp_chardev_validate_socket, that is called from the main > qmp_chardev_open_socket method. This adds a missing check for rejecting > TLS creds with the vsock address type. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------ > 1 file changed, 66 insertions(+), 26 deletions(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index eaa8e8b68f..6669acb35f 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque) > return false; > } > > + > +static bool qmp_chardev_validate_socket(ChardevSocket *sock, > + SocketAddress *addr, > + Error **errp) > +{ > + /* Validate any options which have a dependancy on address type */ > + switch (addr->type) { > + case SOCKET_ADDRESS_TYPE_FD: > + if (sock->has_reconnect) { > + error_setg(errp, > + "'reconnect' option is incompatible with " > + "'fd' address type"); > + return false; > + } > + if (sock->has_tls_creds && > + !(sock->has_server && sock->server)) { > + error_setg(errp, > + "'tls_creds' option is incompatible with " > + "'fd' address type as client"); > + return false; > + } > + break; > + > + case SOCKET_ADDRESS_TYPE_UNIX: > + if (sock->has_tls_creds) { > + error_setg(errp, > + "'tls_creds' option is incompatible with " > + "'unix' address type"); > + return false; > + } > + break; > + > + case SOCKET_ADDRESS_TYPE_INET: > + break; > + > + case SOCKET_ADDRESS_TYPE_VSOCK: > + if (sock->has_tls_creds) { > + error_setg(errp, > + "'tls_creds' option is incompatible with " > + "'vsock' address type"); > + return false; > + } > + > + default: > + break; > + } > + > + /* Validate any options which have a dependancy on client vs server */ > + if (!(sock->has_server && sock->server)) { > + if (sock->has_websocket && sock->websocket) { > + error_setg(errp, "%s", "Websocket client is not implemented"); > + return false; > + } > + } > + > + return true; > +} > + > + > static void qmp_chardev_open_socket(Chardev *chr, > ChardevBackend *backend, > bool *be_opened, > @@ -1004,11 +1063,6 @@ static void qmp_chardev_open_socket(Chardev *chr, > QIOChannelSocket *sioc = NULL; > SocketAddress *addr; > > - if (!is_listen && is_websock) { > - error_setg(errp, "%s", "Websocket client is not implemented"); > - goto error; > - } > - > s->is_listen = is_listen; > s->is_telnet = is_telnet; > s->is_tn3270 = is_tn3270; > @@ -1049,10 +1103,10 @@ static void qmp_chardev_open_socket(Chardev *chr, > > s->addr = addr = socket_address_flatten(sock->addr); > > - if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) { > - error_setg(errp, "'reconnect' option is incompatible with 'fd'"); > + if (!qmp_chardev_validate_socket(sock, addr, errp)) { > goto error; > } > + > qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE); > /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */ > if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) { > @@ -1140,27 +1194,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, > return; > } > > - backend->type = CHARDEV_BACKEND_KIND_SOCKET; > - if (path) { > - if (tls_creds) { > - error_setg(errp, "TLS can only be used over TCP socket"); > - return; > - } > - } else if (host) { > - if (!port) { > - error_setg(errp, "chardev: socket: no port given"); > - return; > - } > - } else if (fd) { > - /* We don't know what host to validate against when in client mode */ > - if (tls_creds && !is_listen) { > - error_setg(errp, "TLS can not be used with pre-opened client FD"); > - return; > - } > - } else { > - g_assert_not_reached(); > + if (host && !port) { > + error_setg(errp, "chardev: socket: no port given"); > + return; > } > > + backend->type = CHARDEV_BACKEND_KIND_SOCKET; > sock = backend->u.socket.data = g_new0(ChardevSocket, 1); > qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock)); > > @@ -1178,6 +1217,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, > sock->wait = is_waitconnect; > sock->has_reconnect = qemu_opt_find(opts, "reconnect"); > sock->reconnect = reconnect; > + sock->has_tls_creds = tls_creds; > sock->tls_creds = g_strdup(tls_creds); > > addr = g_new0(SocketAddressLegacy, 1); > -- > 2.20.1 >
On 2019-01-15 15:52, Daniel P. Berrangé wrote: > The TLS creds option is not valid with certain address types. The user > config was only checked for errors when parsing legacy QemuOpts, thus > the user could pass unsupported values via QMP. > > Pull all code for validating options out into a new method > qmp_chardev_validate_socket, that is called from the main > qmp_chardev_open_socket method. This adds a missing check for rejecting > TLS creds with the vsock address type. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------ > 1 file changed, 66 insertions(+), 26 deletions(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index eaa8e8b68f..6669acb35f 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque) > return false; > } > > + Please remove the additional empty line. > +static bool qmp_chardev_validate_socket(ChardevSocket *sock, > + SocketAddress *addr, > + Error **errp) > +{ > + /* Validate any options which have a dependancy on address type */ I'd maybe rather write "dependency" which is AFAIK the more common spelling - but I'm not a native speaker, so feel free to ignore me here. > + switch (addr->type) { > + case SOCKET_ADDRESS_TYPE_FD: > + if (sock->has_reconnect) { > + error_setg(errp, > + "'reconnect' option is incompatible with " > + "'fd' address type"); > + return false; > + } > + if (sock->has_tls_creds && > + !(sock->has_server && sock->server)) { > + error_setg(errp, > + "'tls_creds' option is incompatible with " > + "'fd' address type as client"); > + return false; > + } > + break; > + > + case SOCKET_ADDRESS_TYPE_UNIX: > + if (sock->has_tls_creds) { > + error_setg(errp, > + "'tls_creds' option is incompatible with " > + "'unix' address type"); > + return false; > + } > + break; > + > + case SOCKET_ADDRESS_TYPE_INET: > + break; You could drop the empty case. > + case SOCKET_ADDRESS_TYPE_VSOCK: > + if (sock->has_tls_creds) { > + error_setg(errp, > + "'tls_creds' option is incompatible with " > + "'vsock' address type"); > + return false; > + } > + > + default: > + break; You could drop the empty default case. > + } > + > + /* Validate any options which have a dependancy on client vs server */ > + if (!(sock->has_server && sock->server)) { > + if (sock->has_websocket && sock->websocket) { > + error_setg(errp, "%s", "Websocket client is not implemented"); > + return false; > + } > + } > + > + return true; > +} > + > + No duplicated empty lines, please. > static void qmp_chardev_open_socket(Chardev *chr, > ChardevBackend *backend, > bool *be_opened, > @@ -1004,11 +1063,6 @@ static void qmp_chardev_open_socket(Chardev *chr, > QIOChannelSocket *sioc = NULL; > SocketAddress *addr; > > - if (!is_listen && is_websock) { > - error_setg(errp, "%s", "Websocket client is not implemented"); > - goto error; > - } > - > s->is_listen = is_listen; > s->is_telnet = is_telnet; > s->is_tn3270 = is_tn3270; > @@ -1049,10 +1103,10 @@ static void qmp_chardev_open_socket(Chardev *chr, > > s->addr = addr = socket_address_flatten(sock->addr); > > - if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) { > - error_setg(errp, "'reconnect' option is incompatible with 'fd'"); > + if (!qmp_chardev_validate_socket(sock, addr, errp)) { > goto error; > } > + > qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE); > /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */ > if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) { > @@ -1140,27 +1194,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, > return; > } > > - backend->type = CHARDEV_BACKEND_KIND_SOCKET; > - if (path) { > - if (tls_creds) { > - error_setg(errp, "TLS can only be used over TCP socket"); > - return; > - } > - } else if (host) { > - if (!port) { > - error_setg(errp, "chardev: socket: no port given"); > - return; > - } > - } else if (fd) { > - /* We don't know what host to validate against when in client mode */ > - if (tls_creds && !is_listen) { > - error_setg(errp, "TLS can not be used with pre-opened client FD"); > - return; > - } > - } else { > - g_assert_not_reached(); > + if (host && !port) { > + error_setg(errp, "chardev: socket: no port given"); > + return; > } > > + backend->type = CHARDEV_BACKEND_KIND_SOCKET; > sock = backend->u.socket.data = g_new0(ChardevSocket, 1); > qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock)); > > @@ -1178,6 +1217,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, > sock->wait = is_waitconnect; > sock->has_reconnect = qemu_opt_find(opts, "reconnect"); > sock->reconnect = reconnect; > + sock->has_tls_creds = tls_creds; > sock->tls_creds = g_strdup(tls_creds); > > addr = g_new0(SocketAddressLegacy, 1); > With at least the redundant empty lines removed: Reviewed-by: Thomas Huth <thuth@redhat.com>
On Wed, Jan 16, 2019 at 06:07:41AM +0100, Thomas Huth wrote: > On 2019-01-15 15:52, Daniel P. Berrangé wrote: > > The TLS creds option is not valid with certain address types. The user > > config was only checked for errors when parsing legacy QemuOpts, thus > > the user could pass unsupported values via QMP. > > > > Pull all code for validating options out into a new method > > qmp_chardev_validate_socket, that is called from the main > > qmp_chardev_open_socket method. This adds a missing check for rejecting > > TLS creds with the vsock address type. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------ > > 1 file changed, 66 insertions(+), 26 deletions(-) > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > > index eaa8e8b68f..6669acb35f 100644 > > --- a/chardev/char-socket.c > > +++ b/chardev/char-socket.c > > @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque) > > return false; > > } > > > > + > > Please remove the additional empty line. Having two blanks lines between functions is intentional to give visual separation. > > +static bool qmp_chardev_validate_socket(ChardevSocket *sock, > > + SocketAddress *addr, > > + Error **errp) > > +{ > > + /* Validate any options which have a dependancy on address type */ > > I'd maybe rather write "dependency" which is AFAIK the more common > spelling - but I'm not a native speaker, so feel free to ignore me here. > > > + switch (addr->type) { > > + case SOCKET_ADDRESS_TYPE_FD: > > + if (sock->has_reconnect) { > > + error_setg(errp, > > + "'reconnect' option is incompatible with " > > + "'fd' address type"); > > + return false; > > + } > > + if (sock->has_tls_creds && > > + !(sock->has_server && sock->server)) { > > + error_setg(errp, > > + "'tls_creds' option is incompatible with " > > + "'fd' address type as client"); > > + return false; > > + } > > + break; > > + > > + case SOCKET_ADDRESS_TYPE_UNIX: > > + if (sock->has_tls_creds) { > > + error_setg(errp, > > + "'tls_creds' option is incompatible with " > > + "'unix' address type"); > > + return false; > > + } > > + break; > > + > > + case SOCKET_ADDRESS_TYPE_INET: > > + break; > > You could drop the empty case. I preferred to explicitly list all cases, so it is clear what needs to be handled here when further checks are added later. > > > + case SOCKET_ADDRESS_TYPE_VSOCK: > > + if (sock->has_tls_creds) { > > + error_setg(errp, > > + "'tls_creds' option is incompatible with " > > + "'vsock' address type"); > > + return false; > > + } > > + Opps, missing default. > > + default: > > + break; > > You could drop the empty default case. If that is not there, then the compiler forces the listing of SOCKET_ADDRESS_TYPE__MAX instead due to -Wswitch Regards, Daniel
Eric, there's a QAPI code generation idea at the end. Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Jan 16, 2019 at 06:07:41AM +0100, Thomas Huth wrote: >> On 2019-01-15 15:52, Daniel P. Berrangé wrote: >> > The TLS creds option is not valid with certain address types. The user >> > config was only checked for errors when parsing legacy QemuOpts, thus >> > the user could pass unsupported values via QMP. >> > >> > Pull all code for validating options out into a new method >> > qmp_chardev_validate_socket, that is called from the main >> > qmp_chardev_open_socket method. This adds a missing check for rejecting >> > TLS creds with the vsock address type. >> > >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >> > --- >> > chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------ >> > 1 file changed, 66 insertions(+), 26 deletions(-) >> > >> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c >> > index eaa8e8b68f..6669acb35f 100644 >> > --- a/chardev/char-socket.c >> > +++ b/chardev/char-socket.c >> > @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque) >> > return false; >> > } >> > >> > + >> >> Please remove the additional empty line. > > Having two blanks lines between functions is intentional to > give visual separation. > >> > +static bool qmp_chardev_validate_socket(ChardevSocket *sock, >> > + SocketAddress *addr, >> > + Error **errp) >> > +{ >> > + /* Validate any options which have a dependancy on address type */ >> >> I'd maybe rather write "dependency" which is AFAIK the more common >> spelling - but I'm not a native speaker, so feel free to ignore me here. For what it's worth, my dictionary wants dependency. >> > + switch (addr->type) { >> > + case SOCKET_ADDRESS_TYPE_FD: >> > + if (sock->has_reconnect) { >> > + error_setg(errp, >> > + "'reconnect' option is incompatible with " >> > + "'fd' address type"); >> > + return false; >> > + } >> > + if (sock->has_tls_creds && >> > + !(sock->has_server && sock->server)) { >> > + error_setg(errp, >> > + "'tls_creds' option is incompatible with " >> > + "'fd' address type as client"); >> > + return false; >> > + } >> > + break; >> > + >> > + case SOCKET_ADDRESS_TYPE_UNIX: >> > + if (sock->has_tls_creds) { >> > + error_setg(errp, >> > + "'tls_creds' option is incompatible with " >> > + "'unix' address type"); >> > + return false; >> > + } >> > + break; >> > + >> > + case SOCKET_ADDRESS_TYPE_INET: >> > + break; >> >> You could drop the empty case. > > I preferred to explicitly list all cases, so it is clear what > needs to be handled here when further checks are added later. Matter of taste, your choice unless maintainer overrules. >> >> > + case SOCKET_ADDRESS_TYPE_VSOCK: >> > + if (sock->has_tls_creds) { >> > + error_setg(errp, >> > + "'tls_creds' option is incompatible with " >> > + "'vsock' address type"); >> > + return false; >> > + } >> > + > > Opps, missing default. I guess you mean break. >> > + default: >> > + break; >> >> You could drop the empty default case. > > If that is not there, then the compiler forces the > listing of SOCKET_ADDRESS_TYPE__MAX instead due > to -Wswitch I wonder whether generating something like typedef enum SocketAddressType { SOCKET_ADDRESS_TYPE_INET, SOCKET_ADDRESS_TYPE_UNIX, SOCKET_ADDRESS_TYPE_VSOCK, SOCKET_ADDRESS_TYPE_FD, } SocketAddressType; #define SOCKET_ADDRESS_TYPE__MAX (SOCKET_ADDRESS_TYPE_FD + 1) would be better.
On 1/17/19 3:21 AM, Markus Armbruster wrote: > Eric, there's a QAPI code generation idea at the end. > >> If that is not there, then the compiler forces the >> listing of SOCKET_ADDRESS_TYPE__MAX instead due >> to -Wswitch > > I wonder whether generating something like > > typedef enum SocketAddressType { > SOCKET_ADDRESS_TYPE_INET, > SOCKET_ADDRESS_TYPE_UNIX, > SOCKET_ADDRESS_TYPE_VSOCK, > SOCKET_ADDRESS_TYPE_FD, > } SocketAddressType; > > #define SOCKET_ADDRESS_TYPE__MAX (SOCKET_ADDRESS_TYPE_FD + 1) > > would be better. Or even an anon enum instead of a define: typedef enum SocketAddressType { SOCKET_ADDRESS_TYPE_INET, SOCKET_ADDRESS_TYPE_UNIX, SOCKET_ADDRESS_TYPE_VSOCK, SOCKET_ADDRESS_TYPE_FD, } SocketAddressType; enum { SOCKET_ADDRESS_TYPE__MAX = SOCKET_ADDRESS_TYPE_FD + 1 }; The idea is interesting; since it is generated code, the change is fairly easy to make. I don't know how much fall-out we'd get in the rest of the code base - only one way to find out.
diff --git a/chardev/char-socket.c b/chardev/char-socket.c index eaa8e8b68f..6669acb35f 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque) return false; } + +static bool qmp_chardev_validate_socket(ChardevSocket *sock, + SocketAddress *addr, + Error **errp) +{ + /* Validate any options which have a dependancy on address type */ + switch (addr->type) { + case SOCKET_ADDRESS_TYPE_FD: + if (sock->has_reconnect) { + error_setg(errp, + "'reconnect' option is incompatible with " + "'fd' address type"); + return false; + } + if (sock->has_tls_creds && + !(sock->has_server && sock->server)) { + error_setg(errp, + "'tls_creds' option is incompatible with " + "'fd' address type as client"); + return false; + } + break; + + case SOCKET_ADDRESS_TYPE_UNIX: + if (sock->has_tls_creds) { + error_setg(errp, + "'tls_creds' option is incompatible with " + "'unix' address type"); + return false; + } + break; + + case SOCKET_ADDRESS_TYPE_INET: + break; + + case SOCKET_ADDRESS_TYPE_VSOCK: + if (sock->has_tls_creds) { + error_setg(errp, + "'tls_creds' option is incompatible with " + "'vsock' address type"); + return false; + } + + default: + break; + } + + /* Validate any options which have a dependancy on client vs server */ + if (!(sock->has_server && sock->server)) { + if (sock->has_websocket && sock->websocket) { + error_setg(errp, "%s", "Websocket client is not implemented"); + return false; + } + } + + return true; +} + + static void qmp_chardev_open_socket(Chardev *chr, ChardevBackend *backend, bool *be_opened, @@ -1004,11 +1063,6 @@ static void qmp_chardev_open_socket(Chardev *chr, QIOChannelSocket *sioc = NULL; SocketAddress *addr; - if (!is_listen && is_websock) { - error_setg(errp, "%s", "Websocket client is not implemented"); - goto error; - } - s->is_listen = is_listen; s->is_telnet = is_telnet; s->is_tn3270 = is_tn3270; @@ -1049,10 +1103,10 @@ static void qmp_chardev_open_socket(Chardev *chr, s->addr = addr = socket_address_flatten(sock->addr); - if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) { - error_setg(errp, "'reconnect' option is incompatible with 'fd'"); + if (!qmp_chardev_validate_socket(sock, addr, errp)) { goto error; } + qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE); /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */ if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) { @@ -1140,27 +1194,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, return; } - backend->type = CHARDEV_BACKEND_KIND_SOCKET; - if (path) { - if (tls_creds) { - error_setg(errp, "TLS can only be used over TCP socket"); - return; - } - } else if (host) { - if (!port) { - error_setg(errp, "chardev: socket: no port given"); - return; - } - } else if (fd) { - /* We don't know what host to validate against when in client mode */ - if (tls_creds && !is_listen) { - error_setg(errp, "TLS can not be used with pre-opened client FD"); - return; - } - } else { - g_assert_not_reached(); + if (host && !port) { + error_setg(errp, "chardev: socket: no port given"); + return; } + backend->type = CHARDEV_BACKEND_KIND_SOCKET; sock = backend->u.socket.data = g_new0(ChardevSocket, 1); qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock)); @@ -1178,6 +1217,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, sock->wait = is_waitconnect; sock->has_reconnect = qemu_opt_find(opts, "reconnect"); sock->reconnect = reconnect; + sock->has_tls_creds = tls_creds; sock->tls_creds = g_strdup(tls_creds); addr = g_new0(SocketAddressLegacy, 1);
The TLS creds option is not valid with certain address types. The user config was only checked for errors when parsing legacy QemuOpts, thus the user could pass unsupported values via QMP. Pull all code for validating options out into a new method qmp_chardev_validate_socket, that is called from the main qmp_chardev_open_socket method. This adds a missing check for rejecting TLS creds with the vsock address type. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 26 deletions(-)