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é <berra...@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 <th...@redhat.com>
