| Commit message (Collapse) | Author | Age | Lines |
|---|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
sizeof() should never be used to get the size of an array. It is very
unsafe, since arrays easily decay to pointers, and sizeof() applied to
a pointer gives false results that compile and produce silent bugs.
It's especially important within macros that will be used in unplanned
cases, where a programmer would just expect it to work, rather than
inline in code where it can be more obvious that some combination is
not a good idea.
An important case where arrays can decay without the programmer
noticing is in the ternary operator (? :). The ternary operator
applies default promotions and other undesired effects to the arguments,
which causes arrays to decay to pointers.
The following expression seems reasonable:
ngx_string(tls ? "https://" : "http://")
And it is not. The code above would be expanded (prior to this patch)
to:
{ sizeof(tls ? "https://" : "http://") - 1,
(u_char *) tls ? "https://" : "http://" }
which evaluates to:
{ sizeof(char *) - 1, (u_char *) tls ? "https://" : "http://" }
which evaluates to:
{ 8 - 1, (u_char *) tls ? "https://" : "http://" }
Of course, a programmer would not want that, but rather:
{ (tls ? 9 : 8) - 1, (u_char *) tls ? "https://" : "http://" }
The worst part in this example is that since one of the strings has
exactly the same size as a pointer in most platforms, testing would
not report an issue in one of the paths of code (coincidentally, the
easier one to test), so it would be very difficult to detect this bug,
either in tests, or in code review.
This example is not a hypothetical one, but rather one that was found
by chance in Nginx Unit. Now, imagine that both strings in the ternary
operator would have 8 bytes: tests would not possibly catch the bug,
and future changes to the code where one of the strings might change
would result in a completely unexpected bug that would be very hard to
track.
This patch makes such code trigger a compile-time warning that prevents
this class of bugs by using this macro.
This is also a recommendation that new code measuring length of arrays
uses the same macro instead of sizeof() directly. A stackoverflow post
linked below details some more recommendations about sizeof() and
arrays.
Link: <https://stackoverflow.com/a/57537491>
Cc: Andrew Clayton <a.clayton@nginx.com>
Cc: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
This macro abstracts calculating how many items are there in an array.
Since recent versions of GCC and Clang provide a warning if we pass a
pointer to the construct in this macro, it is safe to use without any
further checks that the argument is an array.
Link: <https://stackoverflow.com/a/57537491>
Cc: Andrew Clayton <a.clayton@nginx.com>
Cc: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
|
| | |
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
The layer is enabled as a fallback if the QUIC support is configured and the
BoringSSL API wasn't detected, or when using the --with-openssl option, also
compatible with QuicTLS and LibreSSL. For the latter, the layer is assumed
to be present if QUIC was requested, so it needs to be undefined to prevent
QUIC API redefinition as appropriate.
A previously used approach to test the TLSEXT_TYPE_quic_transport_parameters
macro doesn't work with OpenSSL 3.2 master branch where this macro appeared
with incompatible QUIC API. To fix the build there, the test is revised to
pass only for QuicTLS and LibreSSL.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Previously, ngx_quic_close_connection() could be called in a way that QUIC
connection was accessed after the call. In most cases the connection is not
closed right away, but close timeout is scheduled. However, it's not always
the case. Also, if the close process started earlier for a different reason,
calling ngx_quic_close_connection() may actually close the connection. The
connection object should not be accessed after that.
Now, when possible, return statement is added to eliminate post-close connection
object access. In other places ngx_quic_close_connection() is substituted with
posting close event.
Also, the new way of closing connection in ngx_quic_stream_cleanup_handler()
fixes another problem in this function. Previously it passed stream connection
instead of QUIC connection to ngx_quic_close_connection(). This could result
in incomplete connection shutdown. One consequence of that could be that QUIC
streams were freed without shutting down their application contexts. This could
result in another use-after-free.
Found by Coverity (CID 1530402).
|
| |
|
|
|
|
|
|
|
|
| |
The qsock->sockaddr field is a ngx_sockaddr_t union, and therefore can hold
any sockaddr (and union members, such qsock->sockaddr.sockaddr, can be used
to access appropriate variant of the sockaddr). It is better to set it via
qsock->sockaddr itself though, and not qsock->sockaddr.sockaddr, so static
analyzers won't complain about out-of-bounds access.
Prodded by Coverity (CID 1530403).
|
| |\ |
|
| | | |
|
| | | |
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Previously, ngx_udp_rbtree_insert_value() was used for plain UDP and
ngx_quic_rbtree_insert_value() was used for QUIC. Because of this it was
impossible to initialize connection tree in ngx_create_listening() since
this function is not aware what kind of listening it creates.
Now ngx_udp_rbtree_insert_value() is used for both QUIC and UDP. To make
is possible, a generic key field is added to ngx_udp_connection_t. It keeps
client address for UDP and connection ID for QUIC.
|
| | | |
|
| | | |
|
| | |
| |
| |
| | |
The parameter has been deprecated since c851a2ed5ce8.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
The directive used to set the value of the "max_udp_payload_size" transport
parameter. According to RFC 9000, Section 18.2, the value specifies the size
of buffer for reading incoming datagrams:
This limit does act as an additional constraint on datagram size in
the same way as the path MTU, but it is a property of the endpoint
and not the path; see Section 14. It is expected that this is the
space an endpoint dedicates to holding incoming packets.
Current QUIC implementation uses the maximum possible buffer size (65527) for
reading datagrams.
|
| | |
| |
| |
| | |
The value of 65527 is the maximum permitted UDP payload size.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
HTTP and Stream variables $remote_addr and $binary_remote_addr rely on
constant client address, particularly because they are cacheable.
However, QUIC client may migrate to a new address. While there's no perfect
way to handle this, the proposed solution is to copy client address to QUIC
stream at stream creation.
The change also fixes truncated $remote_addr if migration happened while the
stream was active. The reason is addr_text string was copied to stream by
value.
|
| | |
| |
| |
| |
| |
| | |
Previously, the value was not set and remained zero. While in nginx code the
value of c->sockaddr is accessed without taking c->socklen into account,
invalid c->socklen could lead to unexpected results in third-party modules.
|
| | |
| |
| |
| |
| |
| | |
Previously, the post-migration value of addr_text could be truncated, if
it was longer than the previous one. Also, the new value always included
port, which should not be there.
|
| | |
| |
| |
| |
| |
| | |
Two issues fixed:
- new path validation could be scheduled late
- a validated path could leave a spurious timer
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
According to RFC 9000, 8.2.4. Failed Path Validation,
the following value is recommended as a validation timeout:
A value of three times the larger of the current PTO
or the PTO for the new path (using kInitialRtt, as
defined in [QUIC-RECOVERY]) is RECOMMENDED.
The change adds PTO of the new path to the equation as the lower bound.
|
| | |
| |
| |
| |
| |
| |
| |
| | |
Path validation packets containing PATH_CHALLENGE frames are sent separately
from regular frame queue, because of the need to use a decicated path and pad
the packets. The packets are sent periodically, separately from the regular
probe/lost detection mechanism. A path validation packet is resent up to 3
times, each time after PTO expiration, with increasing per-path PTO backoff.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
The check is needed for clients in order to unblock a server due to
anti-amplification limits, and it seems to make no sense for servers.
See RFC 9002, A.6 and A.8 for a further explanation.
This makes max_ack_delay to now always account, notably including
PATH_CHALLENGE timers as noted in the last paragraph of 9000, 9.4,
unlike when it was only used when there are packets in flight.
While here, fixed nearby style.
|
| | |
| |
| |
| |
| |
| | |
As per RFC 9000, Section 14:
UDP datagrams MUST NOT be fragmented at the IP layer.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| | |
Previously, ssl_encryption_application was hardcoded. Before 9553eea74f2a,
ngx_quic_frame_sendto() was used only for PATH_CHALLENGE/PATH_RESPONSE sent
at the application level only. Since 9553eea74f2a, ngx_quic_frame_sendto()
is also used for CONNECTION_CLOSE, which can be sent at initial level after
SSL handshake error or rejection. This resulted in packet encryption error.
Now level is copied from frame, which fixes the error.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Previously, before sending CONNECTION_CLOSE to client, all pending frames
were sent. This is redundant and could prevent CONNECTION_CLOSE from being
sent due to congestion control. Now pending frames are freed and
CONNECTION_CLOSE is sent without congestion control, as advised by RFC 9002:
Packets containing frames besides ACK or CONNECTION_CLOSE frames
count toward congestion control limits and are considered to be in flight.
|
| | |
| |
| |
| |
| |
| |
| | |
Do not corrupt frame data chain pointer on ngx_quic_read_buffer() error.
The error leads to closing a QUIC connection where the frame may be used
as part of the QUIC connection tear down, which envolves writing pending
frames, including this one.
|
| | |
| |
| |
| | |
A QUIC connection is not usable yet at this early stage of spin up.
|
| | |
| |
| |
| | |
TLSv1.3 is enabled by default since d1cf09451ae8.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Previously, waiting on a shared connection was not allowed, because the only
type of such connection was plain UDP. However, QUIC stream connections are
also shared since they share socket descriptor with the listen connection.
Meanwhile, it's perfectly normal to wait on such connections.
The issue manifested itself with stream write errors when the amount of data
exceeded stream buffer size or flow control. Now no error is triggered
and Stream write module is allowed to wait for buffer space to become available.
|
| | | |
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
When a stream is created by client, it's often the case that nginx will send
immediate response on that stream. An example is HTTP/3 request stream, which
in most cases quickly replies with at least HTTP headers.
QUIC stream init handlers are called from a posted event. Output QUIC
frames are also sent to client from a posted event, called the push event.
If the push event is posted before the stream init event, then output produced
by stream may trigger sending an extra UDP datagram. To address this, push
event is now re-posted when a new stream init event is posted.
An example is handling 0-RTT packets. Client typically sends an init packet
coalesced with a 0-RTT packet. Previously, nginx replied with a padded CRYPTO
datagram, followed by a 1-RTT stream reply datagram. Now CRYPTO and STREAM
packets are coalesced in one reply datagram, which saves bandwidth.
Other examples include coalescing 1-RTT first stream response, and
MAX_STREAMS/STREAM sent in response to ACK/STREAM.
|
| | |\ |
|
| | | | |
|
| | | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
SSL context is not present if the default server has neither certificates nor
ssl_reject_handshake enabled. Previously, this led to null pointer dereference
before it would be caught with configuration checks.
Additionally, non-default servers with distinct SSL contexts need to initialize
compatibility layer in order to complete a QUIC handshake.
|
| | | |
| | |
| | |
| | |
| | | |
Now "ssl", "proxy_protocol" and "http2" are not allowed with "quic" in "listen"
directive. Previously, only "ssl" was not allowed.
|
| | | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
Now "listen" directve has a new "quic" parameter which enables QUIC protocol
for the address. Further, to enable HTTP/3, a new directive "http3" is
introduced. The hq-interop protocol is enabled by "http3_hq" as before.
Now application protocol is chosen by ALPN.
Previously used "http3" parameter of "listen" is deprecated.
|
| | | |
| | |
| | |
| | |
| | |
| | | |
The change allows to compile QUIC with OpenSSL which lacks BoringSSL QUIC API.
This implementation does not support 0-RTT.
|
| | | |
| | |
| | |
| | | |
The check follows the ngx_ssl_handshake() change in 59e1c73fe02b.
|
| | | | |
|
| | | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
A QUIC handshake failure breaks down into several cases:
- a handshake error which leads to a send_alert call
- an error triggered by the add_handshake_data callback
- internal errors (allocation etc)
Previously, in the first case, only error code was set in the send_alert
callback. Now the "handshake failed" reason phrase is set there as well.
In the second case, both code and reason are set by add_handshake_data.
In the last case, setting reason phrase is removed: returning NGX_ERROR
now leads to closing the connection with just INTERNAL_ERROR.
Reported by Jiuzhou Cui.
|
| | | |
| | |
| | |
| | | |
Patch by Jiuzhou Cui.
|
| | | | |
|
| | | |
| | |
| | |
| | | |
While here, updated link to mailman.
|
| | | | |
|
| | | |
| | |
| | |
| | |
| | |
| | | |
Previously, since 3550b00d9dc8, the token was allocated on stack, to get
rid of pool usage. Now the token is allocated by ngx_quic_copy_buffer()
in QUIC buffers, also used for STREAM, CRYPTO and ACK frames.
|
| | | |
| | |
| | |
| | |
| | | |
The function copies passed data to QUIC buffer chain and returns it.
The chain can be used in ngx_quic_frame_t data field.
|
| | | |
| | |
| | |
| | |
| | | |
In nginx source code the inttypes.h include, if available, is used to define
standard integer types. Changed the SO_COOKIE configure test to follow this.
|
| | | |
| | |
| | |
| | |
| | |
| | |
| | | |
Specifically, now it is kept unset until streams are initialized.
Notably, this unbreaks OCSP with client certificates after 35e27117b593.
Previously, the read event could be posted prematurely via ngx_quic_set_event()
e.g., as part of handling a STREAM frame.
|
| | | |
| | |
| | |
| | |
| | |
| | |
| | | |
Previously, streams were initialized in early keys handler. However, client
transport parameters may not be available by then. This happens, for example,
when using QuicTLS. Now streams are initialized in ngx_quic_crypto_input()
after calling SSL_do_handshake() for both 0-RTT and 1-RTT.
|
