QUIC: ngx_quic_set_error() function. #708
Conversation
|
Fixed BoringSSL build. |
|
Examples of logging a handshake crypto error. BoringSSL: |
| ngx_quic_set_error(c, NGX_QUIC_ERR_TRANSPORT_PARAMETER_ERROR, | ||
| "invalid initial_source_connection_id"); |
There was a problem hiding this comment.
This contradicts the "client sent invalid ..." idiom used for logging client errors.
| @@ -520,9 +504,9 @@ ngx_quic_close_connection(ngx_connection_t *c, ngx_int_t rc) | |||
| * to terminate the connection immediately. | |||
| */ | |||
|
|
|||
| if (qc->error == 0 && rc == NGX_ERROR) { | |||
| qc->error = NGX_QUIC_ERR_INTERNAL_ERROR; | |||
| qc->error_app = 0; | |||
There was a problem hiding this comment.
I don't see how
qc->error_appis now reset.
Now errors are never reset after being set once. So there's no need to reset error_app,.
There was a problem hiding this comment.
qc->error_app does not need to be reset. If qc->error in unchanged, then qc->error_app is unchanged as well.
|
|
||
| qc = ngx_quic_get_connection(c); | ||
|
|
||
| if (qc->error) { |
There was a problem hiding this comment.
if (qc->error != NGX_QUIC_ERR_NO_ERROR) ?
There was a problem hiding this comment.
What's the difference? We treat 0 as the default value and do no direct initialiazation of qc->error.
There was a problem hiding this comment.
Somehow I overlooked the code line in a hurry, no need to change this.
The comment is actually about comparing err with 0 slightly below.
For style reasons, it could be changed to if (err == NGX_QUIC_ERR_NO_ERROR)
just because after that you compare it with real errors.
src/event/quic/ngx_event_quic.c
Outdated
| } | ||
|
|
||
| if (err >= NGX_QUIC_ERR_CRYPTO_ERROR | ||
| && err <= NGX_QUIC_ERR_LAST_CRYPTO_ERROR) |
There was a problem hiding this comment.
maybe, define NGX_QUIC_ERR_LAST_CRYPTO_ERROR as the first invalid value (0x200) and test for < here?
There was a problem hiding this comment.
Indeed we use a similar notation with NGX_HTTP_LAST_2XX, NGX_HTTP_LAST_3XX etc. It will be confusing though if they'll come up with an actual error 0x200 in future versions.
| @@ -109,6 +109,7 @@ | |||
| #define NGX_QUIC_ERR_NO_VIABLE_PATH 0x10 | |||
|
|
|||
| #define NGX_QUIC_ERR_CRYPTO_ERROR 0x100 | |||
| #define NGX_QUIC_ERR_LAST_CRYPTO_ERROR 0x1ff | |||
There was a problem hiding this comment.
It may be a good idea to shorten the name to NGX_QUIC_ERR_CRYPTO_LAST
There was a problem hiding this comment.
Not sure if it's a good idea. Also, see the http macros above for reference.
There was a problem hiding this comment.
I'm afraid NGX_HTTP_LAST_2XX is not a good example.
The reasoning behind this is to keep a common prefix with NGX_QUIC_ERR_CRYPTO_ERROR.
Well, I won't insist, it's up to you.
| if (qc->error == 0) { | ||
| qc->error_ftype = frame.type; | ||
| } |
There was a problem hiding this comment.
I have to admit that this change (in particular, to propagate the NGX_QUIC_FT_ACK value) is quite counterintuitive, it may deserve a separate commit with explanation.
| @@ -621,6 +605,47 @@ ngx_quic_close_connection(ngx_connection_t *c, ngx_int_t rc) | |||
| } | |||
|
|
|||
|
|
|||
| void | |||
| ngx_quic_set_error(ngx_connection_t *c, ngx_uint_t err, const char *reason) | |||
There was a problem hiding this comment.
It doesn't only sets, but also logs.
So why not ngx_quic_error().
There was a problem hiding this comment.
I suggest we switch to just setting at this point. I will switch logging to debug and give it some though.
Previously qc->error_ftype was only set in ngx_quic_handle_ack_frame_range() while handling an ACK frame and ignored in other frame handlers. Meanwhile RFC 9000 specifies that a CONNECTION_CLOSE frame should carry the frame type which triggered the error. To simplify setting error frame type, qc->error_ftype is now set automatically to the currently processed frame type. If no error was generated while handling a frame, the value is reset.
|
Made frame type change a separate patch. Also, removed error logging from Here's simple grep from h3 tests: |
The function sets QUIC error unless it was set before. Previously, a new error always reset the old error. Now the original error stays unchanged for logging and sending to client.
The function sets QUIC error unless it was set before. After that the error is logged. Cryptographic handshake errors are properly formatted.
The change allows to simplify error generation in the QUIC code. Also, it eliminates the necessity to explicitly log QUIC errors. Each call of ngx_quic_set_error() now receives a well-formatted error message that is both logged and later sent to the client.
Additionally, qc->error_ftype is now detetced automatically based on the currently processed frame.