Hello!
On Tue, Nov 15, 2022 at 04:28:30PM +0400, Sergey Kandaurov wrote:
>
> > On 21 Oct 2022, at 04:10, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Tue, Oct 11, 2022 at 02:35:52PM +0400, Sergey Kandaurov wrote:
> >
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet@nginx.com>
> >> # Date 1665484414 -14400
> >> # Tue Oct 11 14:33:34 2022 +0400
> >> # Branch quic
> >> # Node ID c0165ddcb1c6981f8e5230081f03a277f62d20c3
> >> # Parent caced81ce0a9cb218ae8cdd6176c12e0614acee9
> >> QUIC: support for setting QUIC methods with LibreSSL.
> >>
> >> Setting QUIC methods is converted to use C99 designated initializers
> >> for simplicity, as LibreSSL 3.6.0 has different SSL_QUIC_METHOD layout.
> >
> > I'm somewhat sceptical about C99 designated initializers. These
> > aren't supported by MSVC till 2012: in particular, this includes
> > all MSVC versions available via wineticks, as well as MSVC
> > versions currently used to build nginx win32 binaries.
> >
> > A more portable solution might be to use run-time initialization
> > instead.
>
> While I agree in principle, fixing build with MSVC would require
> a larger work to implement support for UDP connections on win32.
> Specifically, nginx-quic uses c->udp->buffer and sockets API that
> needs porting to win32, see ngx_quic_send() / ngx_quic_recvmsg().
This shouldn't be a major problem, given the relevant APIs are
available on Windows.
But the point is: there are no reasons to introduce additional
issues in advance, we'll have to fix this sooner or later.
> Other than that, currently the build fails in other places
> as seen with MSVC 2010. Below the patches to address this.
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1668509694 -14400
> # Tue Nov 15 14:54:54 2022 +0400
> # Branch quic
> # Node ID 322f11b1272fd3a0b67fc4cc29b12b6c09e64c6f
> # Parent 572a9a4a0642514fa4ef1d8c8121dfb14cadf7c0
> QUIC: fixed C4706 warnings with MSVC 2010.
>
> The fix is to avoid assignments within conditional expression.
>
> diff --git a/src/event/quic/ngx_event_quic_transport.c b/src/event/quic/ngx_event_quic_transport.c
> --- a/src/event/quic/ngx_event_quic_transport.c
> +++ b/src/event/quic/ngx_event_quic_transport.c
> @@ -804,11 +804,23 @@ ngx_quic_parse_frame(ngx_quic_header_t *
> case NGX_QUIC_FT_ACK:
> case NGX_QUIC_FT_ACK_ECN:
>
> - if (!((p = ngx_quic_parse_int(p, end, &f->u.ack.largest))
> - && (p = ngx_quic_parse_int(p, end, &f->u.ack.delay))
> - && (p = ngx_quic_parse_int(p, end, &f->u.ack.range_count))
> - && (p = ngx_quic_parse_int(p, end, &f->u.ack.first_range))))
> - {
> + p = ngx_quic_parse_int(p, end, &f->u.ack.largest);
> + if (p == NULL) {
> + goto error;
> + }
> +
> + p = ngx_quic_parse_int(p, end, &f->u.ack.delay);
> + if (p == NULL) {
> + goto error;
> + }
> +
> + p = ngx_quic_parse_int(p, end, &f->u.ack.range_count);
> + if (p == NULL) {
> + goto error;
> + }
> +
> + p = ngx_quic_parse_int(p, end, &f->u.ack.first_range);
> + if (p == NULL) {
> goto error;
> }
>
> @@ -818,10 +830,11 @@ ngx_quic_parse_frame(ngx_quic_header_t *
> for (i = 0; i < f->u.ack.range_count; i++) {
>
> p = ngx_quic_parse_int(p, end, &varint);
> - if (p) {
> - p = ngx_quic_parse_int(p, end, &varint);
> + if (p == NULL) {
> + goto error;
> }
>
> + p = ngx_quic_parse_int(p, end, &varint);
> if (p == NULL) {
> goto error;
> }
> @@ -833,10 +846,18 @@ ngx_quic_parse_frame(ngx_quic_header_t *
>
> if (f->type == NGX_QUIC_FT_ACK_ECN) {
>
> - if (!((p = ngx_quic_parse_int(p, end, &f->u.ack.ect0))
> - && (p = ngx_quic_parse_int(p, end, &f->u.ack.ect1))
> - && (p = ngx_quic_parse_int(p, end, &f->u.ack.ce))))
> - {
> + p = ngx_quic_parse_int(p, end, &f->u.ack.ect0);
> + if (p == NULL) {
> + goto error;
> + }
> +
> + p = ngx_quic_parse_int(p, end, &f->u.ack.ect1);
> + if (p == NULL) {
> + goto error;
> + }
> +
> + p = ngx_quic_parse_int(p, end, &f->u.ack.ce);
> + if (p == NULL) {
> goto error;
> }
>
> @@ -989,11 +1010,18 @@ ngx_quic_parse_frame(ngx_quic_header_t *
>
> case NGX_QUIC_FT_RESET_STREAM:
>
> - if (!((p = ngx_quic_parse_int(p, end, &f->u.reset_stream.id))
> - && (p = ngx_quic_parse_int(p, end, &f->u.reset_stream.error_code))
> - && (p = ngx_quic_parse_int(p, end,
> - &f->u.reset_stream.final_size))))
> - {
> + p = ngx_quic_parse_int(p, end, &f->u.reset_stream.id);
> + if (p == NULL) {
> + goto error;
> + }
> +
> + p = ngx_quic_parse_int(p, end, &f->u.reset_stream.error_code);
> + if (p == NULL) {
> + goto error;
> + }
> +
> + p = ngx_quic_parse_int(p, end, &f->u.reset_stream.final_size);
> + if (p == NULL) {
> goto error;
> }
>
Looks good.
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1668510614 -14400
> # Tue Nov 15 15:10:14 2022 +0400
> # Branch quic
> # Node ID eff714f6d07aa595b0f4f552b76a228b5dd5758f
> # Parent 322f11b1272fd3a0b67fc4cc29b12b6c09e64c6f
> QUIC: moved variable declaration to fix build with MSVC 2010.
>
> Previously, ngx_quic_hkdf_t variables used declaration with assignment
> in the middle of a function, which is not supported by MSVC 2010.
> Fixing this also required to rewrite the ngx_quic_hkdf_set macro
> and to switch to an explicit array size.
>
> diff --git a/src/event/quic/ngx_event_quic_protection.c b/src/event/quic/ngx_event_quic_protection.c
> --- a/src/event/quic/ngx_event_quic_protection.c
> +++ b/src/event/quic/ngx_event_quic_protection.c
> @@ -48,12 +48,10 @@ typedef struct {
> const u_char *label;
> } ngx_quic_hkdf_t;
>
> -#define ngx_quic_hkdf_set(label, out, prk) \
> - { \
> - (out)->len, (out)->data, \
> - (prk)->len, (prk)->data, \
> - (sizeof(label) - 1), (u_char *)(label), \
> - }
> +#define ngx_quic_hkdf_set(seq, _label, _out, _prk) \
> + (seq)->out_len = (_out)->len; (seq)->out = (_out)->data; \
> + (seq)->prk_len = (_prk)->len, (seq)->prk = (_prk)->data, \
> + (seq)->label_len = (sizeof(_label) - 1); (seq)->label = (u_char *)(_label);
>
>
> static ngx_int_t ngx_hkdf_expand(u_char *out_key, size_t out_len,
> @@ -151,6 +149,7 @@ ngx_quic_keys_set_initial_secret(ngx_qui
> uint8_t is[SHA256_DIGEST_LENGTH];
> ngx_uint_t i;
> const EVP_MD *digest;
> + ngx_quic_hkdf_t seq[8];
> ngx_quic_secret_t *client, *server;
>
> static const uint8_t salt[20] =
> @@ -203,17 +202,15 @@ ngx_quic_keys_set_initial_secret(ngx_qui
> client->iv.len = NGX_QUIC_IV_LEN;
> server->iv.len = NGX_QUIC_IV_LEN;
>
> - ngx_quic_hkdf_t seq[] = {
> - /* labels per RFC 9001, 5.1. Packet Protection Keys */
> - ngx_quic_hkdf_set("tls13 client in", &client->secret, &iss),
> - ngx_quic_hkdf_set("tls13 quic key", &client->key, &client->secret),
> - ngx_quic_hkdf_set("tls13 quic iv", &client->iv, &client->secret),
> - ngx_quic_hkdf_set("tls13 quic hp", &client->hp, &client->secret),
> - ngx_quic_hkdf_set("tls13 server in", &server->secret, &iss),
> - ngx_quic_hkdf_set("tls13 quic key", &server->key, &server->secret),
> - ngx_quic_hkdf_set("tls13 quic iv", &server->iv, &server->secret),
> - ngx_quic_hkdf_set("tls13 quic hp", &server->hp, &server->secret),
> - };
> + /* labels per RFC 9001, 5.1. Packet Protection Keys */
> + ngx_quic_hkdf_set(&seq[0], "tls13 client in", &client->secret, &iss);
> + ngx_quic_hkdf_set(&seq[1], "tls13 quic key", &client->key, &client->secret);
> + ngx_quic_hkdf_set(&seq[2], "tls13 quic iv", &client->iv, &client->secret);
> + ngx_quic_hkdf_set(&seq[3], "tls13 quic hp", &client->hp, &client->secret);
> + ngx_quic_hkdf_set(&seq[4], "tls13 server in", &server->secret, &iss);
> + ngx_quic_hkdf_set(&seq[5], "tls13 quic key", &server->key, &server->secret);
> + ngx_quic_hkdf_set(&seq[6], "tls13 quic iv", &server->iv, &server->secret);
> + ngx_quic_hkdf_set(&seq[7], "tls13 quic hp", &server->hp, &server->secret);
>
> for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
> if (ngx_quic_hkdf_expand(&seq[i], digest, log) != NGX_OK) {
> @@ -639,6 +636,7 @@ ngx_quic_keys_set_encryption_secret(ngx_
> ngx_int_t key_len;
> ngx_str_t secret_str;
> ngx_uint_t i;
> + ngx_quic_hkdf_t seq[3];
> ngx_quic_secret_t *peer_secret;
> ngx_quic_ciphers_t ciphers;
>
> @@ -670,11 +668,10 @@ ngx_quic_keys_set_encryption_secret(ngx_
> secret_str.len = secret_len;
> secret_str.data = (u_char *) secret;
>
> - ngx_quic_hkdf_t seq[] = {
> - ngx_quic_hkdf_set("tls13 quic key", &peer_secret->key, &secret_str),
> - ngx_quic_hkdf_set("tls13 quic iv", &peer_secret->iv, &secret_str),
> - ngx_quic_hkdf_set("tls13 quic hp", &peer_secret->hp, &secret_str),
> - };
> + ngx_quic_hkdf_set(&seq[0], "tls13 quic key",
> + &peer_secret->key, &secret_str);
> + ngx_quic_hkdf_set(&seq[1], "tls13 quic iv", &peer_secret->iv, &secret_str);
> + ngx_quic_hkdf_set(&seq[2], "tls13 quic hp", &peer_secret->hp, &secret_str);
>
> for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
> if (ngx_quic_hkdf_expand(&seq[i], ciphers.d, log) != NGX_OK) {
> @@ -720,6 +717,7 @@ ngx_int_t
> ngx_quic_keys_update(ngx_connection_t *c, ngx_quic_keys_t *keys)
> {
> ngx_uint_t i;
> + ngx_quic_hkdf_t seq[6];
> ngx_quic_ciphers_t ciphers;
> ngx_quic_secrets_t *current, *next;
>
> @@ -744,20 +742,18 @@ ngx_quic_keys_update(ngx_connection_t *c
> next->server.iv.len = NGX_QUIC_IV_LEN;
> next->server.hp = current->server.hp;
>
> - ngx_quic_hkdf_t seq[] = {
> - ngx_quic_hkdf_set("tls13 quic ku",
> - &next->client.secret, ¤t->client.secret),
> - ngx_quic_hkdf_set("tls13 quic key",
> - &next->client.key, &next->client.secret),
> - ngx_quic_hkdf_set("tls13 quic iv",
> - &next->client.iv, &next->client.secret),
> - ngx_quic_hkdf_set("tls13 quic ku",
> - &next->server.secret, ¤t->server.secret),
> - ngx_quic_hkdf_set("tls13 quic key",
> - &next->server.key, &next->server.secret),
> - ngx_quic_hkdf_set("tls13 quic iv",
> - &next->server.iv, &next->server.secret),
> - };
> + ngx_quic_hkdf_set(&seq[0], "tls13 quic ku",
> + &next->client.secret, ¤t->client.secret);
> + ngx_quic_hkdf_set(&seq[1], "tls13 quic key",
> + &next->client.key, &next->client.secret);
> + ngx_quic_hkdf_set(&seq[2], "tls13 quic iv",
> + &next->client.iv, &next->client.secret);
> + ngx_quic_hkdf_set(&seq[3], "tls13 quic ku",
> + &next->server.secret, ¤t->server.secret);
> + ngx_quic_hkdf_set(&seq[4], "tls13 quic key",
> + &next->server.key, &next->server.secret);
> + ngx_quic_hkdf_set(&seq[5], "tls13 quic iv",
> + &next->server.iv, &next->server.secret);
>
> for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
> if (ngx_quic_hkdf_expand(&seq[i], ciphers.d, c->log) != NGX_OK) {
Looks good.
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1668515080 -14400
> # Tue Nov 15 16:24:40 2022 +0400
> # Branch quic
> # Node ID bec3d3cdc80388fb71204df2cf778c8d9f2a0321
> # Parent eff714f6d07aa595b0f4f552b76a228b5dd5758f
> QUIC: avoid using C99 designated initializers.
>
> They are not supported by MSVC till 2012.
>
> SSL_QUIC_METHOD initialization is moved to run-time to preserve portability
> among SSL library implementations, which allows to reduce its visibility.
> Note using of a static storage to keep SSL_set_quic_method() reference valid.
>
> diff --git a/src/event/quic/ngx_event_quic_protection.c b/src/event/quic/ngx_event_quic_protection.c
> --- a/src/event/quic/ngx_event_quic_protection.c
> +++ b/src/event/quic/ngx_event_quic_protection.c
> @@ -147,6 +147,7 @@ ngx_quic_keys_set_initial_secret(ngx_qui
> {
> size_t is_len;
> uint8_t is[SHA256_DIGEST_LENGTH];
> + ngx_str_t iss;
> ngx_uint_t i;
> const EVP_MD *digest;
> ngx_quic_hkdf_t seq[8];
> @@ -176,10 +177,8 @@ ngx_quic_keys_set_initial_secret(ngx_qui
> return NGX_ERROR;
> }
>
> - ngx_str_t iss = {
> - .data = is,
> - .len = is_len
> - };
> + iss.len = is_len;
> + iss.data = is;
>
> ngx_log_debug0(NGX_LOG_DEBUG_EVENT, log, 0,
> "quic ngx_quic_set_initial_secret");
> diff --git a/src/event/quic/ngx_event_quic_ssl.c b/src/event/quic/ngx_event_quic_ssl.c
> --- a/src/event/quic/ngx_event_quic_ssl.c
> +++ b/src/event/quic/ngx_event_quic_ssl.c
> @@ -39,19 +39,6 @@ static int ngx_quic_send_alert(ngx_ssl_c
> static ngx_int_t ngx_quic_crypto_input(ngx_connection_t *c, ngx_chain_t *data);
>
>
> -static SSL_QUIC_METHOD quic_method = {
> -#if defined OPENSSL_IS_BORINGSSL || defined LIBRESSL_VERSION_NUMBER
> - .set_read_secret = ngx_quic_set_read_secret,
> - .set_write_secret = ngx_quic_set_write_secret,
> -#else
> - .set_encryption_secrets = ngx_quic_set_encryption_secrets,
> -#endif
> - .add_handshake_data = ngx_quic_add_handshake_data,
> - .flush_flight = ngx_quic_flush_flight,
> - .send_alert = ngx_quic_send_alert,
> -};
> -
> -
> #if defined OPENSSL_IS_BORINGSSL || defined LIBRESSL_VERSION_NUMBER
>
> static int
> @@ -533,13 +520,14 @@ ngx_quic_crypto_input(ngx_connection_t *
> ngx_int_t
> ngx_quic_init_connection(ngx_connection_t *c)
> {
> - u_char *p;
> - size_t clen;
> - ssize_t len;
> - ngx_str_t dcid;
> - ngx_ssl_conn_t *ssl_conn;
> - ngx_quic_socket_t *qsock;
> - ngx_quic_connection_t *qc;
> + u_char *p;
> + size_t clen;
> + ssize_t len;
> + ngx_str_t dcid;
> + ngx_ssl_conn_t *ssl_conn;
> + ngx_quic_socket_t *qsock;
> + ngx_quic_connection_t *qc;
> + static SSL_QUIC_METHOD quic_method;
>
> qc = ngx_quic_get_connection(c);
>
> @@ -551,6 +539,18 @@ ngx_quic_init_connection(ngx_connection_
>
> ssl_conn = c->ssl->connection;
>
> + if (!quic_method.send_alert) {
> +#if defined OPENSSL_IS_BORINGSSL || defined LIBRESSL_VERSION_NUMBER
> + quic_method.set_read_secret = ngx_quic_set_read_secret;
> + quic_method.set_write_secret = ngx_quic_set_write_secret;
> +#else
> + quic_method.set_encryption_secrets = ngx_quic_set_encryption_secrets;
> +#endif
> + quic_method.add_handshake_data = ngx_quic_add_handshake_data;
> + quic_method.flush_flight = ngx_quic_flush_flight;
> + quic_method.send_alert = ngx_quic_send_alert;
> + }
> +
> if (SSL_set_quic_method(ssl_conn, &quic_method) == 0) {
> ngx_log_error(NGX_LOG_INFO, c->log, 0,
> "quic SSL_set_quic_method() failed");
Looks good.
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1668513723 -14400
> # Tue Nov 15 16:02:03 2022 +0400
> # Branch quic
> # Node ID 42a9a02b402a44a48cef2b35e09e7fbec7ee8e1e
> # Parent 004338233abf98def885a35ea465349d190eb19b
> QUIC: fixed C4389 MSVC warning about signed/unsigned mismatch.
>
> diff --git a/src/event/quic/ngx_event_quic_protection.c b/src/event/quic/ngx_event_quic_protection.c
> --- a/src/event/quic/ngx_event_quic_protection.c
> +++ b/src/event/quic/ngx_event_quic_protection.c
> @@ -988,8 +988,9 @@ ngx_quic_decrypt(ngx_quic_header_t *pkt,
> u_char *p, *sample;
> size_t len;
> uint64_t pn, lpn;
> - ngx_int_t pnl, rc, key_phase;
> + ngx_int_t pnl, rc;
> ngx_str_t in, ad;
> + ngx_uint_t key_phase;
> ngx_quic_secret_t *secret;
> ngx_quic_ciphers_t ciphers;
> uint8_t nonce[NGX_QUIC_IV_LEN], mask[NGX_QUIC_HP_LEN];
Looks good.
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1668513724 -14400
> # Tue Nov 15 16:02:04 2022 +0400
> # Branch quic
> # Node ID d4f3464f71f03a28d16484f861057be03ffdd72a
> # Parent 42a9a02b402a44a48cef2b35e09e7fbec7ee8e1e
> Added shutdown macros for win32 required for QUIC.
>
> diff --git a/src/os/win32/ngx_socket.h b/src/os/win32/ngx_socket.h
> --- a/src/os/win32/ngx_socket.h
> +++ b/src/os/win32/ngx_socket.h
> @@ -14,6 +14,8 @@
>
>
> #define NGX_WRITE_SHUTDOWN SD_SEND
> +#define NGX_READ_SHUTDOWN SD_RECEIVE
> +#define NGX_RDWR_SHUTDOWN SD_BOTH
>
>
> typedef SOCKET ngx_socket_t;
Looks good.
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1668513728 -14400
> # Tue Nov 15 16:02:08 2022 +0400
> # Branch quic
> # Node ID d760c2e49d4c4647621765fef3404c34d1aef81b
> # Parent d4f3464f71f03a28d16484f861057be03ffdd72a
> QUIC: plug MSVC warning about potentially uninitialized variable.
>
> diff --git a/src/event/quic/ngx_event_quic_tokens.c b/src/event/quic/ngx_event_quic_tokens.c
> --- a/src/event/quic/ngx_event_quic_tokens.c
> +++ b/src/event/quic/ngx_event_quic_tokens.c
> @@ -178,6 +178,10 @@ ngx_quic_validate_token(ngx_connection_t
> u_char addr_hash[20];
> u_char tdec[NGX_QUIC_MAX_TOKEN_SIZE];
>
> +#if NGX_SUPPRESS_WARN
> + ngx_str_null(&odcid);
> +#endif
> +
> /* Retry token or NEW_TOKEN in a previous connection */
>
> cipher = EVP_aes_256_cbc();
Looks good.
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1668513998 -14400
> # Tue Nov 15 16:06:38 2022 +0400
> # Branch quic
> # Node ID 1ba7992c7bb0f801e069adb15b3378b5211a85a8
> # Parent d760c2e49d4c4647621765fef3404c34d1aef81b
> QUIC: silenced C4334 MSVC warning about 32 to 64 bits convertion.
converSion?
>
> diff --git a/src/event/quic/ngx_event_quic_ack.c b/src/event/quic/ngx_event_quic_ack.c
> --- a/src/event/quic/ngx_event_quic_ack.c
> +++ b/src/event/quic/ngx_event_quic_ack.c
> @@ -195,7 +195,7 @@ ngx_quic_rtt_sample(ngx_connection_t *c,
> } else {
> qc->min_rtt = ngx_min(qc->min_rtt, latest_rtt);
>
> - ack_delay = ack->delay * (1 << qc->ctp.ack_delay_exponent) / 1000;
> + ack_delay = ack->delay * (1ULL << qc->ctp.ack_delay_exponent) / 1000;
>
> if (c->ssl->handshaked) {
> ack_delay = ngx_min(ack_delay, qc->ctp.max_ack_delay);
Why "1 << ..." at all?
Shouldn't it be
diff -r d9ef59e283e3 src/event/quic/ngx_event_quic_ack.c
--- a/src/event/quic/ngx_event_quic_ack.c Tue Nov 15 16:02:08 2022 +0400
+++ b/src/event/quic/ngx_event_quic_ack.c Fri Nov 18 06:55:05 2022 +0300
@@ -195,7 +195,7 @@ ngx_quic_rtt_sample(ngx_connection_t *c,
} else {
qc->min_rtt = ngx_min(qc->min_rtt, latest_rtt);
- ack_delay = ack->delay * (1 << qc->ctp.ack_delay_exponent) / 1000;
+ ack_delay = (ack->delay << qc->ctp.ack_delay_exponent) / 1000;
if (c->ssl->handshaked) {
ack_delay = ngx_min(ack_delay, qc->ctp.max_ack_delay);
instead?
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
