[{"id":1764296,"web_url":"http://patchwork.ozlabs.org/comment/1764296/","msgid":"","list_archive_url":null,"date":"2017-09-06T18:06:03","subject":"Re: [Qemu-devel] [PATCH v1 1/2] io: send proper HTTP response for\n\twebsocket errors","submitter":{"id":70924,"url":"http://patchwork.ozlabs.org/api/people/70924/","name":"Philippe Mathieu-Daudé","email":"f4bug@amsat.org"},"content":"Hi Daniel,\n\nOn 09/06/2017 07:40 AM, Daniel P. Berrange wrote:\n> When any error occurs while processing the websockets handshake,\n> QEMU just terminates the connection abruptly. This is in violation\n> of the HTTP specs and does not help the client understand what they\n> did wrong. This is particularly bad when the client gives the wrong\n> path, as a \"404 Not Found\" would be very helpful.\n> \n> Refactor the handshake code so that it always sends a response to\n> the client unless there was an I/O error.\n> \n> Fixes bug: #1715186\n> \n> Signed-off-by: Daniel P. Berrange \n> ---\n> io/channel-websock.c | 179 ++++++++++++++++++++++++++++++++++++++-------------\n> 1 file changed, 134 insertions(+), 45 deletions(-)\n> \n> diff --git a/io/channel-websock.c b/io/channel-websock.c\n> index 5a3badbec2..b9cc5a1371 100644\n> --- a/io/channel-websock.c\n> +++ b/io/channel-websock.c\n> @@ -44,13 +44,39 @@\n> #define QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE \"Upgrade\"\n> #define QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET \"websocket\"\n> \n> -#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE \\\n> +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> + \"Server: QEMU VNC\\r\\n\" \\\n> + \"Date: %s\\r\\n\"\n\nand\n \"Sec-WebSocket-Version: \" \\\n QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION \"\\r\\n\" \\\n\nor what about:\n\n#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON(conn) \\\n \"Server: QEMU VNC\\r\\n\" \\\n \"Date: %s\\r\\n\" \\\n \"Connection: \" conn \"\\r\\n\" \\\n \"Sec-WebSocket-Version: \" \\\n QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION \"\\r\\n\"\n> +\n> +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK \\\n> \"HTTP/1.1 101 Switching Protocols\\r\\n\" \\\n> + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> \"Upgrade: websocket\\r\\n\" \\\n> \"Connection: Upgrade\\r\\n\" \\\n> \"Sec-WebSocket-Accept: %s\\r\\n\" \\\n> \"Sec-WebSocket-Protocol: binary\\r\\n\" \\\n> \"\\r\\n\"\n> +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND \\\n> + \"HTTP/1.1 404 Not Found\\r\\n\" \\\n> + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> + \"Connection: close\\r\\n\" \\\n> + \"\\r\\n\"\n> +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \\\n> + \"HTTP/1.1 400 Bad Request\\r\\n\" \\\n> + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> + \"Connection: close\\r\\n\" \\\n> + \"Sec-WebSocket-Version: 13\\r\\n\" \\\n\ndrop\n\n> + \"\\r\\n\"\n\nor with previous macro:\n\n#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \\\n \"HTTP/1.1 400 Bad Request\\r\\n\" \\\n QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON(\"close\") \\\n \"\\r\\n\"\n\n> +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR \\\n> + \"HTTP/1.1 500 Internal Server Error\\r\\n\" \\\n> + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> + \"Connection: close\\r\\n\" \\\n> + \"\\r\\n\"\n> +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE \\\n> + \"HTTP/1.1 403 Request Entity Too Large\\r\\n\" \\\n> + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> + \"Connection: close\\r\\n\" \\\n> + \"\\r\\n\"\n> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM \"\\r\\n\"\n> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END \"\\r\\n\\r\\n\"\n> #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION \"13\"\n> @@ -123,8 +149,43 @@ enum {\n> QIO_CHANNEL_WEBSOCK_OPCODE_PONG = 0xA\n> };\n> \n> +static void qio_channel_websock_handshake_send_res(QIOChannelWebsock *ioc,\n> + const char *resmsg,\n> + ...)\n> +{\n> + va_list vargs;\n> + char *response = NULL;\n\nNULL not needed\n\n> + size_t responselen;\n> +\n> + va_start(vargs, resmsg);\n> + response = g_strdup_vprintf(resmsg, vargs);\n> + responselen = strlen(response);\n> + buffer_reserve(&ioc->encoutput, responselen);\n> + buffer_append(&ioc->encoutput, response, responselen);\n> + va_end(vargs);\n> +}\n> +\n> +static gchar *qio_channel_websock_date_str(void)\n> +{\n> + GTimeZone *utc = g_time_zone_new_utc();\n> + GDateTime *now = g_date_time_new_now(utc);\n> + gchar *ret = g_date_time_format(now, \"%a, %d %b %Y %H:%M:%S GMT\");\n\nassert(ret);\n\n> + g_date_time_unref(now);\n> + g_time_zone_unref(utc);\n> + return ret;\n> +}\n> +\n> +static void qio_channel_websock_handshake_send_res_err(QIOChannelWebsock *ioc,\n> + const char *resdata)\n> +{\n> + char *date = qio_channel_websock_date_str();\n> + qio_channel_websock_handshake_send_res(ioc, resdata, date);\n> + g_free(date);\n> +}\n> +\n> static size_t\n> -qio_channel_websock_extract_headers(char *buffer,\n> +qio_channel_websock_extract_headers(QIOChannelWebsock *ioc,\n> + char *buffer,\n> QIOChannelWebsockHTTPHeader *hdrs,\n> size_t nhdrsalloc,\n> Error **errp)\n> @@ -145,7 +206,7 @@ qio_channel_websock_extract_headers(char *buffer,\n> nl = strstr(buffer, QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM);\n> if (!nl) {\n> error_setg(errp, \"Missing HTTP header delimiter\");\n> - return 0;\n> + goto bad_request;\n> }\n> *nl = '\\0';\n> \n> @@ -158,18 +219,20 @@ qio_channel_websock_extract_headers(char *buffer,\n> \n> if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_METHOD)) {\n> error_setg(errp, \"Unsupported HTTP method %s\", buffer);\n> - return 0;\n> + goto bad_request;\n> }\n> \n> buffer = tmp + 1;\n> tmp = strchr(buffer, ' ');\n> if (!tmp) {\n> error_setg(errp, \"Missing HTTP version delimiter\");\n> - return 0;\n> + goto bad_request;\n> }\n> *tmp = '\\0';\n> \n> if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_PATH)) {\n> + qio_channel_websock_handshake_send_res_err(\n> + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND);\n> error_setg(errp, \"Unexpected HTTP path %s\", buffer);\n> return 0;\n> }\n> @@ -178,7 +241,7 @@ qio_channel_websock_extract_headers(char *buffer,\n> \n> if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_VERSION)) {\n> error_setg(errp, \"Unsupported HTTP version %s\", buffer);\n> - return 0;\n> + goto bad_request;\n> }\n> \n> buffer = nl + strlen(QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM);\n> @@ -203,7 +266,7 @@ qio_channel_websock_extract_headers(char *buffer,\n> sep = strchr(buffer, ':');\n> if (!sep) {\n> error_setg(errp, \"Malformed HTTP header\");\n> - return 0;\n> + goto bad_request;\n> }\n> *sep = '\\0';\n> sep++;\n> @@ -213,7 +276,7 @@ qio_channel_websock_extract_headers(char *buffer,\n> \n> if (nhdrs >= nhdrsalloc) {\n> error_setg(errp, \"Too many HTTP headers\");\n> - return 0;\n> + goto bad_request;\n> }\n> \n> hdr = &hdrs[nhdrs++];\n> @@ -231,6 +294,11 @@ qio_channel_websock_extract_headers(char *buffer,\n> } while (nl != NULL);\n> \n> return nhdrs;\n> +\n> + bad_request:\n> + qio_channel_websock_handshake_send_res_err(\n> + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST);\n> + return 0;\n> }\n> \n> static const char *\n> @@ -250,14 +318,14 @@ qio_channel_websock_find_header(QIOChannelWebsockHTTPHeader *hdrs,\n> }\n> \n> \n> -static int qio_channel_websock_handshake_send_response(QIOChannelWebsock *ioc,\n> - const char *key,\n> - Error **errp)\n> +static void qio_channel_websock_handshake_send_res_ok(QIOChannelWebsock *ioc,\n> + const char *key,\n> + Error **errp)\n> {\n> char combined_key[QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN +\n> QIO_CHANNEL_WEBSOCK_GUID_LEN + 1];\n> - char *accept = NULL, *response = NULL;\n> - size_t responselen;\n> + char *accept = NULL;\n> + char *date = qio_channel_websock_date_str();\n> \n> g_strlcpy(combined_key, key, QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN + 1);\n> g_strlcat(combined_key, QIO_CHANNEL_WEBSOCK_GUID,\n> @@ -271,105 +339,110 @@ static int qio_channel_websock_handshake_send_response(QIOChannelWebsock *ioc,\n> QIO_CHANNEL_WEBSOCK_GUID_LEN,\n> &accept,\n> errp) < 0) {\n> - return -1;\n> + qio_channel_websock_handshake_send_res_err(\n> + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR);\n> + return;\n> }\n> \n> - response = g_strdup_printf(QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE, accept);\n> - responselen = strlen(response);\n> - buffer_reserve(&ioc->encoutput, responselen);\n> - buffer_append(&ioc->encoutput, response, responselen);\n> + qio_channel_websock_handshake_send_res(\n> + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK, date, accept);\n> \n> + g_free(date);\n> g_free(accept);\n> - g_free(response);\n> \n> - return 0;\n> + return;\n\ndrop\n\n> }\n> \n> -static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,\n> - char *buffer,\n> - Error **errp)\n> +static void qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,\n> + char *buffer,\n> + Error **errp)\n> {\n> QIOChannelWebsockHTTPHeader hdrs[32];\n> size_t nhdrs = G_N_ELEMENTS(hdrs);\n> const char *protocols = NULL, *version = NULL, *key = NULL,\n> *host = NULL, *connection = NULL, *upgrade = NULL;\n> \n> - nhdrs = qio_channel_websock_extract_headers(buffer, hdrs, nhdrs, errp);\n> + nhdrs = qio_channel_websock_extract_headers(ioc, buffer, hdrs, nhdrs, errp);\n> if (!nhdrs) {\n> - return -1;\n> + return;\n> }\n> \n> protocols = qio_channel_websock_find_header(\n> hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL);\n> if (!protocols) {\n> error_setg(errp, \"Missing websocket protocol header data\");\n> - return -1;\n> + goto bad_request;\n> }\n> \n> version = qio_channel_websock_find_header(\n> hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_VERSION);\n> if (!version) {\n> error_setg(errp, \"Missing websocket version header data\");\n> - return -1;\n> + goto bad_request;\n> }\n> \n> key = qio_channel_websock_find_header(\n> hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_KEY);\n> if (!key) {\n> error_setg(errp, \"Missing websocket key header data\");\n> - return -1;\n> + goto bad_request;\n> }\n> \n> host = qio_channel_websock_find_header(\n> hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_HOST);\n> if (!host) {\n> error_setg(errp, \"Missing websocket host header data\");\n> - return -1;\n> + goto bad_request;\n> }\n> \n> connection = qio_channel_websock_find_header(\n> hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_CONNECTION);\n> if (!connection) {\n> error_setg(errp, \"Missing websocket connection header data\");\n> - return -1;\n> + goto bad_request;\n> }\n> \n> upgrade = qio_channel_websock_find_header(\n> hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_UPGRADE);\n> if (!upgrade) {\n> error_setg(errp, \"Missing websocket upgrade header data\");\n> - return -1;\n> + goto bad_request;\n> }\n> \n> if (!g_strrstr(protocols, QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY)) {\n> error_setg(errp, \"No '%s' protocol is supported by client '%s'\",\n> QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY, protocols);\n> - return -1;\n> + goto bad_request;\n> }\n> \n> if (!g_str_equal(version, QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION)) {\n> error_setg(errp, \"Version '%s' is not supported by client '%s'\",\n> QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION, version);\n> - return -1;\n> + goto bad_request;\n> }\n> \n> if (strlen(key) != QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN) {\n> error_setg(errp, \"Key length '%zu' was not as expected '%d'\",\n> strlen(key), QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN);\n> - return -1;\n> + goto bad_request;\n> }\n> \n> if (!g_strrstr(connection, QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE)) {\n> error_setg(errp, \"No connection upgrade requested '%s'\", connection);\n> - return -1;\n> + goto bad_request;\n> }\n> \n> if (!g_str_equal(upgrade, QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET)) {\n> error_setg(errp, \"Incorrect upgrade method '%s'\", upgrade);\n> - return -1;\n> + goto bad_request;\n> }\n> \n> - return qio_channel_websock_handshake_send_response(ioc, key, errp);\n> + qio_channel_websock_handshake_send_res_ok(ioc, key, errp);\n> + return;\n> +\n> + bad_request:\n> + qio_channel_websock_handshake_send_res_err(\n> + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST);\n> }\n> \n> static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,\n> @@ -393,20 +466,20 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,\n> QIO_CHANNEL_WEBSOCK_HANDSHAKE_END);\n> if (!handshake_end) {\n> if (ioc->encinput.offset >= 4096) {\n> + qio_channel_websock_handshake_send_res_err(\n> + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE);\n> error_setg(errp,\n> \"End of headers not found in first 4096 bytes\");\n> - return -1;\n> + return 1;\n> } else {\n> return 0;\n> }\n> }\n> *handshake_end = '\\0';\n> \n> - if (qio_channel_websock_handshake_process(ioc,\n> - (char *)ioc->encinput.buffer,\n> - errp) < 0) {\n> - return -1;\n> - }\n> + qio_channel_websock_handshake_process(ioc,\n> + (char *)ioc->encinput.buffer,\n> + errp);\n> \n> buffer_advance(&ioc->encinput,\n> handshake_end - (char *)ioc->encinput.buffer +\n> @@ -438,8 +511,15 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,\n> \n> buffer_advance(&wioc->encoutput, ret);\n> if (wioc->encoutput.offset == 0) {\n> - trace_qio_channel_websock_handshake_complete(ioc);\n> - qio_task_complete(task);\n> + if (wioc->io_err) {\n> + trace_qio_channel_websock_handshake_fail(ioc);\n> + qio_task_set_error(task, wioc->io_err);\n> + wioc->io_err = NULL;\n> + qio_task_complete(task);\n> + } else {\n> + trace_qio_channel_websock_handshake_complete(ioc);\n> + qio_task_complete(task);\n> + }\n> return FALSE;\n> }\n> trace_qio_channel_websock_handshake_pending(ioc, G_IO_OUT);\n> @@ -458,6 +538,11 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,\n> \n> ret = qio_channel_websock_handshake_read(wioc, &err);\n> if (ret < 0) {\n> + /*\n> + * We only take this path on a fatal I/O error reading from\n> + * client connection, as most of the time we have an\n> + * HTTP 4xx err response to send instead\n> + */\n> trace_qio_channel_websock_handshake_fail(ioc);\n> qio_task_set_error(task, err);\n> qio_task_complete(task);\n> @@ -469,6 +554,10 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,\n> return TRUE;\n> }\n> \n> + if (err) {\n> + error_propagate(&wioc->io_err, err);\n> + }\n> +\n> trace_qio_channel_websock_handshake_reply(ioc);\n> qio_channel_add_watch(\n> wioc->master,\n> \n\nlooks good :)\n\nRegards,\n\nPhil.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"Oflhv9jo\"; dkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnWkF40qKz9rxl\n\tfor ;\n\tThu, 7 Sep 2017 04:06:44 +1000 (AEST)","from localhost ([::1]:37366 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpein-0005nh-Ka\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Sep 2017 14:06:41 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:46237)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from )\n\tid 1dpeiM-0005lX-Rp\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 14:06:16 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from )\n\tid 1dpeiI-0006QL-Gi\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 14:06:14 -0400","from mail-qk0-x243.google.com ([2607:f8b0:400d:c09::243]:38631)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpeiI-0006Ok-Ao\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 14:06:10 -0400","by mail-qk0-x243.google.com with SMTP id c69so4244661qke.5\n\tfor ; Wed, 06 Sep 2017 11:06:08 -0700 (PDT)","from [192.168.1.10] ([181.93.89.178])\n\tby smtp.gmail.com with ESMTPSA id\n\tt10sm295258qtt.36.2017.09.06.11.06.05\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tWed, 06 Sep 2017 11:06:07 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=sender:subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=d7JlitFk9RIbdAs4N77FAHdw8chQtuNgIPYgCwbCMjc=;\n\tb=Oflhv9joV2QKxzXJKCLAhHth2GOY5geWf0JaxRKFOdp4OSQ9Uiva6ZnOHV0XlrBor2\n\t+VB9vXnWVHNTRsVqNv/eau33PX+VUkWZqF31Igu8UUapFyeqyRCnkOSnHrsdRJO5Ufw3\n\tszWVxvl3Aq1p8D3n5BsoRN61rV8JlM3k7xXUIeyfVyv9AomyFKJvBJpY5oiSKFGkaDst\n\toIO8iJzXXvC38mN8OckhfGB5K5O5BjWI6qOIJ66FgpYWEWOV+1WfFsd2otX/ui0Sk9O5\n\tKQYtuqZK7J1fUt8RH7R/9y3WjoKwc7Px95agz6ufoJB8G8ve9Me7waRNfqc+Y/jXwJeL\n\tdNtg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:sender:subject:to:cc:references:from:message-id\n\t:date:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=d7JlitFk9RIbdAs4N77FAHdw8chQtuNgIPYgCwbCMjc=;\n\tb=nkavc0A+po2iKyUjCHhh97o01CW3P2zmUVKnmMUPgz3nXZfhVo0d9PhpEMgi3rPd2+\n\tueON+1VuNZRmjPDgHDyCGcClvts/H/k71IZcZHkeiSAlhANFD31MsFyjXG81KIzbleNo\n\tWkSpNhumtHRyDYqO0/ZmwDuvJuHqWc3AsUV2PX5kNqNXL3mKvksiePswinnaHQBcKVOg\n\t4u7JvvGjmMNrSkDVGCTlcTE2mwt9k8bGxbGs18nRSzDYZ6BxcoFb6sGdKc1HS8UDI2/G\n\tAXT4FVEPNWGka3vVJkyZtKmsCPSm+43KLlaiCcPzdOsIAKZd+i9MVZo5H9dt2GftBcL+\n\t3Slw==","X-Gm-Message-State":"AHPjjUhwmjn72WUKH3jgB/f9pOvE9OHZXKgE/HIAZwO4na49M/xWBvm9\n\t48NGG4nkvAe6OQ==","X-Google-Smtp-Source":"AOwi7QAYIeuYxpR3ercXmm0YP+z+H/ue0bvzaFOQ2VmW8Fo+1rLhSdHeYXugT2BOk0Xo2+SyA6DDjw==","X-Received":"by 10.233.220.199 with SMTP id q190mr9117qkf.222.1504721168067; \n\tWed, 06 Sep 2017 11:06:08 -0700 (PDT)","To":"\"Daniel P. Berrange\" , qemu-devel@nongnu.org","References":"<20170906104043.30745-1-berrange@redhat.com>\n\t<20170906104043.30745-2-berrange@redhat.com>","From":"=?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= ","Message-ID":"","Date":"Wed, 6 Sep 2017 15:06:03 -0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170906104043.30745-2-berrange@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2607:f8b0:400d:c09::243","Subject":"Re: [Qemu-devel] [PATCH v1 1/2] io: send proper HTTP response for\n\twebsocket errors","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Brian Rak ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1764600,"web_url":"http://patchwork.ozlabs.org/comment/1764600/","msgid":"<20170907091333.GB30609@redhat.com>","list_archive_url":null,"date":"2017-09-07T09:13:33","subject":"Re: [Qemu-devel] [PATCH v1 1/2] io: send proper HTTP response for\n\twebsocket errors","submitter":{"id":2694,"url":"http://patchwork.ozlabs.org/api/people/2694/","name":"Daniel P. Berrangé","email":"berrange@redhat.com"},"content":"On Wed, Sep 06, 2017 at 03:06:03PM -0300, Philippe Mathieu-Daudé wrote:\n> Hi Daniel,\n> \n> On 09/06/2017 07:40 AM, Daniel P. Berrange wrote:\n> > When any error occurs while processing the websockets handshake,\n> > QEMU just terminates the connection abruptly. This is in violation\n> > of the HTTP specs and does not help the client understand what they\n> > did wrong. This is particularly bad when the client gives the wrong\n> > path, as a \"404 Not Found\" would be very helpful.\n> > \n> > Refactor the handshake code so that it always sends a response to\n> > the client unless there was an I/O error.\n> > \n> > Fixes bug: #1715186\n> > \n> > Signed-off-by: Daniel P. Berrange \n> > ---\n> > io/channel-websock.c | 179 ++++++++++++++++++++++++++++++++++++++-------------\n> > 1 file changed, 134 insertions(+), 45 deletions(-)\n> > \n> > diff --git a/io/channel-websock.c b/io/channel-websock.c\n> > index 5a3badbec2..b9cc5a1371 100644\n> > --- a/io/channel-websock.c\n> > +++ b/io/channel-websock.c\n> > @@ -44,13 +44,39 @@\n> > #define QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE \"Upgrade\"\n> > #define QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET \"websocket\"\n> > -#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE \\\n> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> > + \"Server: QEMU VNC\\r\\n\" \\\n> > + \"Date: %s\\r\\n\"\n> \n> and\n> \"Sec-WebSocket-Version: \" \\\n> QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION \"\\r\\n\" \\\n\nThis header is only supposed to be set in error responses,\nnot in the 101 response.\n\n> or what about:\n> \n> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON(conn) \\\n> \"Server: QEMU VNC\\r\\n\" \\\n> \"Date: %s\\r\\n\" \\\n> \"Connection: \" conn \"\\r\\n\" \\\n> \"Sec-WebSocket-Version: \" \\\n> QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION \"\\r\\n\"\n\nI'm not a fan of parameterizing this - I think it is clearer\nto see the full connection header inline below.\n\n> > +\n> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK \\\n> > \"HTTP/1.1 101 Switching Protocols\\r\\n\" \\\n> > + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> > \"Upgrade: websocket\\r\\n\" \\\n> > \"Connection: Upgrade\\r\\n\" \\\n> > \"Sec-WebSocket-Accept: %s\\r\\n\" \\\n> > \"Sec-WebSocket-Protocol: binary\\r\\n\" \\\n> > \"\\r\\n\"\n> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND \\\n> > + \"HTTP/1.1 404 Not Found\\r\\n\" \\\n> > + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> > + \"Connection: close\\r\\n\" \\\n> > + \"\\r\\n\"\n> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \\\n> > + \"HTTP/1.1 400 Bad Request\\r\\n\" \\\n> > + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> > + \"Connection: close\\r\\n\" \\\n> > + \"Sec-WebSocket-Version: 13\\r\\n\" \\\n> \n> drop\n> \n> > + \"\\r\\n\"\n> \n> or with previous macro:\n> \n> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \\\n> \"HTTP/1.1 400 Bad Request\\r\\n\" \\\n> QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON(\"close\") \\\n> \"\\r\\n\"\n> \n> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR \\\n> > + \"HTTP/1.1 500 Internal Server Error\\r\\n\" \\\n> > + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> > + \"Connection: close\\r\\n\" \\\n> > + \"\\r\\n\"\n> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE \\\n> > + \"HTTP/1.1 403 Request Entity Too Large\\r\\n\" \\\n> > + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \\\n> > + \"Connection: close\\r\\n\" \\\n> > + \"\\r\\n\"\n> > #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM \"\\r\\n\"\n> > #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END \"\\r\\n\\r\\n\"\n> > #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION \"13\"\n\n\nRegards,\nDaniel","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=berrange@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnvsN40vDz9sNV\n\tfor ;\n\tThu, 7 Sep 2017 19:14:16 +1000 (AEST)","from localhost ([::1]:39475 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpst4-00076P-Me\n\tfor incoming@patchwork.ozlabs.org; Thu, 07 Sep 2017 05:14:14 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:54635)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpssa-0006yr-MN\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 05:13:46 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpssV-0004TS-Ju\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 05:13:44 -0400","from mx1.redhat.com ([209.132.183.28]:58836)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpssV-0004TC-As\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 05:13:39 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 56E83356D0;\n\tThu, 7 Sep 2017 09:13:38 +0000 (UTC)","from redhat.com (unknown [10.33.36.82])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id CB5565D6A8;\n\tThu, 7 Sep 2017 09:13:36 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 56E83356D0","Date":"Thu, 7 Sep 2017 10:13:33 +0100","From":"\"Daniel P. Berrange\" ","To":"Philippe =?utf-8?q?Mathieu-Daud=C3=A9?= ","Message-ID":"<20170907091333.GB30609@redhat.com>","References":"<20170906104043.30745-1-berrange@redhat.com>\n\t<20170906104043.30745-2-berrange@redhat.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.30]);\n\tThu, 07 Sep 2017 09:13:38 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v1 1/2] io: send proper HTTP response for\n\twebsocket errors","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"\"Daniel P. Berrange\" ","Cc":"Brian Rak , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]