Re: [PATCH] Fixed 413 custom error page for HTTP/2 and HTTP/3 (ticket #2609)
Hello!
On Mon, Mar 04, 2024 at 06:46:23PM +0400, Roman Arutyunyan wrote:
> # HG changeset patch
> # User Roman Arutyunyan
> # Date 1709563405 -14400
> # Mon Mar 04 18:43:25 2024 +0400
> # Node ID 3b0be477ab7246caba4c5152286b8be520ee0418
> # Parent 44da04c2d4db94ad4eefa84b299e07c5fa4a00b9
> Fixed 413 custom error page for HTTP/2 and HTTP/3 (ticket #2609).
>
> Previously an attempt to return a custom 413 error page for these protocols
> resulted in the standard 413 page (if recursive_error_pages was off) or
> otherwise internal redirection cycle followed by the 500 error.
>
> Discarding request body for HTTP/1 starts by setting r->discard_body which
> indicates the body is currently being discarded. If and when the entire body
> is read and discarded, the flag is cleared and r->headers_in.content_length_n
> is set to zero. Both r->discard_body and r->headers_in.content_length_n
> prevent nginx from re-generating 413 error after internal redirect in
> ngx_http_core_find_config_phase().
>
> However the above does not work for HTTP/2 and HTTP/3. Discarding request
> body for these protocols does not affect the above mentioned fields, which is
> why there's no protection against re-generating the 413 error. The fix is to
> assign zero to r->headers_in.content_length_n much like in HTTP/1 case after
> the body is entirely read and discarded, except for these protocols no active
> discard is needed.
>
> diff --git a/src/http/ngx_http_request_body.c
> b/src/http/ngx_http_request_body.c
> --- a/src/http/ngx_http_request_body.c
> +++ b/src/http/ngx_http_request_body.c
> @@ -640,12 +640,14 @@ ngx_http_discard_request_body(ngx_http_r
> #if (NGX_HTTP_V2)
> if (r->stream) {
> r->stream->skip_data = 1;
> +r->headers_in.content_length_n = 0;
> return NGX_OK;
> }
> #endif
>
> #if (NGX_HTTP_V3)
> if (r->http_version == NGX_HTTP_VERSION_30) {
> +r->headers_in.content_length_n = 0;
> return NGX_OK;
> }
> #endif
The patch is wrong, see here:
https://trac.nginx.org/nginx/ticket/1152#comment:6
The issue is in my TODO list. Once properly fixed, you'll be able
to merge the fix from freenginx.
Alternatively, consider submitting patches to
the nginx-de...@freenginx.org list for proper review.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: announcing freenginx.org
Hello! On Thu, Feb 15, 2024 at 04:49:10PM +0800, Archimedes Gaviola wrote: > On Thu, Feb 15, 2024 at 2:03 AM Maxim Dounin wrote: > > > Hello! > > > > As you probably know, F5 closed Moscow office in 2022, and I no > > longer work for F5 since then. Still, we’ve reached an agreement > > that I will maintain my role in nginx development as a volunteer. > > And for almost two years I was working on improving nginx and > > making it better for everyone, for free. > > > > Unfortunately, some new non-technical management at F5 recently > > decided that they know better how to run open source projects. In > > particular, they decided to interfere with security policy nginx > > uses for years, ignoring both the policy and developers’ position. > > > > That’s quite understandable: they own the project, and can do > > anything with it, including doing marketing-motivated actions, > > ignoring developers position and community. Still, this > > contradicts our agreement. And, more importantly, I no longer able > > to control which changes are made in nginx within F5, and no longer > > see nginx as a free and open source project developed and > > maintained for the public good. > > > > As such, starting from today, I will no longer participate in nginx > > development as run by F5. Instead, I’m starting an alternative > > project, which is going to be run by developers, and not corporate > > entities: > > > > http://freenginx.org/ > > > > The goal is to keep nginx development free from arbitrary corporate > > actions. Help and contributions are welcome. Hope it will be > > beneficial for everyone. > > > > > > -- > > Maxim Dounin > > http://freenginx.org/ > > Hi Maxim, > > Sorry to hear that. Is the license still the same for freenginx? Yes, the license will remain the same. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: announcing freenginx.org
Hello! On Thu, Feb 15, 2024 at 01:33:08AM +0300, Vasiliy Soshnikov wrote: > Hello Maxim, > Sad to read it. I can't promise that, but I will try to support your > project by using freenginx at least. > I wish good luck to freenginx! Thanks, appreciated. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: announcing freenginx.org
Hello! On Wed, Feb 14, 2024 at 10:45:37PM +0100, Sergey Brester wrote: > Hi Maxim, > > it is pity to hear such news... > > I have few comments and questions about, which I enclosed inline below... > > Regards, > Serg. > > 14.02.2024 19:03, Maxim Dounin wrote: > > > Hello! > > > > As you probably know, F5 closed Moscow office in 2022, and I no > > longer work for F5 since then. Still, we've reached an agreement > > that I will maintain my role in nginx development as a volunteer. > > And for almost two years I was working on improving nginx and > > making it better for everyone, for free. > > And you did a very good job! Thanks. > > Unfortunately, some new non-technical management at F5 recently > > decided that they know better how to run open source projects. In > > particular, they decided to interfere with security policy nginx > > uses for years, ignoring both the policy and developers' position. > > Can you explain a bit more about that (or provide some examples > or a link to a public discussion about, if it exists)? I've already provided some details here: https://freenginx.org/pipermail/nginx/2024-February/07.html : The most recent "security advisory" was released despite the fact : that the particular bug in the experimental HTTP/3 code is : expected to be fixed as a normal bug as per the existing security : policy, and all the developers, including me, agree on this. : : And, while the particular action isn't exactly very bad, the : approach in general is quite problematic. There was no public discussion. The only discussion I'm aware of happened on the security-alert@ list, and the consensus was that the bug should be fixed as a normal bug. Still, I was reached several days ago with the information that some unnamed management requested an advisory and security release anyway, regardless of the policy and developers position. > > That's quite understandable: they own the project, and can do > > anything with it, including doing marketing-motivated actions, > > ignoring developers position and community. Still, this > > contradicts our agreement. And, more importantly, I no longer able > > to control which changes are made in nginx within F5, and no longer > > see nginx as a free and open source project developed and > > maintained for the public good. > > Do you speak only about you?.. Or are there also other developers which > share your point of view? Just for the record... > What is about R. Arutyunyan, V. Bartenev and others? > Could one expect any statement from Igor (Sysoev) about the subject? I speak only about me. Others, if they are interested in, are welcome to join. > > As such, starting from today, I will no longer participate in nginx > > development as run by F5. Instead, I'm starting an alternative > > project, which is going to be run by developers, and not corporate > > entities: > > > > http://freenginx.org/ [1] > > Why yet another fork? I mean why just not "angie", for instance? The "angie" fork shares the same problem as nginx run by F5: it's run by a for-profit corporate entity. Even if it's good enough now, things might change unexpectedly, like it happened with F5. > Additionally I'd like to ask whether the name "freenginx" is really well > thought-out? > I mean: > - it can be easy confused with free nginx (compared to nginx plus) > - the search for that will be horrible (if you would try to search for > freenginx, > even as exact (within quotes, with plus etc), many internet search > engine > would definitely include free nginx in the result. > - possibly copyright or trademark problems, etc Apart from potential trademark concerns (which I believe do not apply here, but IANAL), these does not seem to be significant (and search results are already good enough). Still, the name aligns well with project goals. > > The goal is to keep nginx development free from arbitrary corporate > > actions. Help and contributions are welcome. Hope it will be > > beneficial for everyone. > > Just as an idea: switch the primary dev to GH (github)... (and commonly from > hg to git). > I'm sure it would boost the development drastically, as well as bring many > new > developers and let grow the community. While I understand the suggestion and potential benefits, I'm not a fun of git and github, and prefer Mercurial. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
announcing freenginx.org
Hello! As you probably know, F5 closed Moscow office in 2022, and I no longer work for F5 since then. Still, we’ve reached an agreement that I will maintain my role in nginx development as a volunteer. And for almost two years I was working on improving nginx and making it better for everyone, for free. Unfortunately, some new non-technical management at F5 recently decided that they know better how to run open source projects. In particular, they decided to interfere with security policy nginx uses for years, ignoring both the policy and developers’ position. That’s quite understandable: they own the project, and can do anything with it, including doing marketing-motivated actions, ignoring developers position and community. Still, this contradicts our agreement. And, more importantly, I no longer able to control which changes are made in nginx within F5, and no longer see nginx as a free and open source project developed and maintained for the public good. As such, starting from today, I will no longer participate in nginx development as run by F5. Instead, I’m starting an alternative project, which is going to be run by developers, and not corporate entities: http://freenginx.org/ The goal is to keep nginx development free from arbitrary corporate actions. Help and contributions are welcome. Hope it will be beneficial for everyone. -- Maxim Dounin http://freenginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Upstream response time populated even when upstream connection timed out.
Hello! On Fri, Feb 09, 2024 at 12:15:54PM +, Sambhav Gupta via nginx-devel wrote: > I have noticed that upstream response time is populated to a > non-zero and non-negative value in Nginx even when upstream > connection times out. > > This is not true with other values like upstream connect time > and upstream header time. > > Is this intentional ? Yes. The $upstream_response_time variable shows total time spent when working with the upstream, with all the activities included. It is always set as long as nginx was working with an upstream, even if there was an error or a timeout. In contrast, $upstream_connect_time and $upstream_header_time are only set when the connection was established or the header was successfully received, respectively. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Segfault when interpreting cached X-Accel-Redirect response
Hello!
On Tue, Feb 06, 2024 at 01:36:20PM +0100, Jan Prachař wrote:
> Hello,
>
> I have one last note bellow.
>
> On Tue, 2024-02-06 at 14:08 +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Tue, Feb 06, 2024 at 11:42:40AM +0100, Jan Prachař wrote:
> >
> > > Hello Maxim,
> > >
> > > On Tue, 2024-02-06 at 00:46 +0300, Maxim Dounin wrote:
> > > > Hello!
> > > >
> > > > On Mon, Feb 05, 2024 at 06:01:54PM +0100, Jan Prachař wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > thank you for your responses.
> > > > >
> > > > > On Sat, 2024-02-03 at 04:25 +0300, Maxim Dounin wrote:
> > > > > > Hello!
> > > > > >
> > > > > > On Fri, Feb 02, 2024 at 01:47:51PM +0100, Jan Prachař wrote:
> > > > > >
> > > > > > > On Fri, 2024-02-02 at 12:48 +0100, Jiří Setnička via nginx-devel
> > > > > > > wrote:
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > Also, I believe that the core of the problem is because of the
> > > > > > > > ngx_http_finalize_request(r, NGX_DONE); call in the
> > > > > > > > ngx_http_upstream_process_headers function. This call is needed
> > > > > > > > when
> > > > > > > > doing an internal redirect after the real upstream request (to
> > > > > > > > close the
> > > > > > > > upstream request), but when serving from the cache, there is no
> > > > > > > > upstream
> > > > > > > > request to close and this call causes
> > > > > > > > ngx_http_set_lingering_close to be
> > > > > > > > called from the ngx_http_finalize_connection with no active
> > > > > > > > request on
> > > > > > > > the connection yielding to the segfault.
> > > > > > >
> > > > > > > Hello,
> > > > > > >
> > > > > > > I am Jiří's colleague, and so I have taken a closer look at the
> > > > > > > problem. Another
> > > > > > > indication of the issue is the alert in the error log for
> > > > > > > non-keepalive connections,
> > > > > > > stating "http request count is zero while closing request."
> > > > > > >
> > > > > > > Upon reviewing the nginx source code, I discovered that the
> > > > > > > function
> > > > > > > ngx_http_upstream_finalize_request(), when called with rc =
> > > > > > > NGX_DECLINED, does not invoke
> > > > > > > ngx_http_finalize_request(). However, when there is nothing to
> > > > > > > clean up (u->cleanup ==
> > > > > > > NULL), it does. Therefore, I believe the appropriate fix is to
> > > > > > > follow the patch below.
> > > > > > >
> > > > > > > Best, Jan Prachař
> > > > > > >
> > > > > > > # User Jan Prachař
> > > > > > > # Date 1706877176 -3600
> > > > > > > # Fri Feb 02 13:32:56 2024 +0100
> > > > > > > # Node ID 851c994b48c48c9cd3d32b9aa402f4821aeb8bb2
> > > > > > > # Parent cf3d537ec6706f8713a757df256f2cfccb8f9b01
> > > > > > > Upstream: Fix "request count is zero" when procesing
> > > > > > > X-Accel-Redirect
> > > > > > >
> > > > > > > ngx_http_upstream_finalize_request(r, u, NGX_DECLINED) should not
> > > > > > > call
> > > > > > > ngx_http_finalize_request().
> > > > > > >
> > > > > > > diff -r cf3d537ec670 -r 851c994b48c4 src/http/ngx_http_upstream.c
> > > > > > > --- a/src/http/ngx_http_upstream.c Thu Nov 26 21:00:25 2020
> > > > > > > +0100
> > > > > > > +++ b/src/http/ngx_http_upstream.c Fri Feb 02 13:32:56 2024
> > > > > > > +0100
> > > > > > > @@ -4340,6 +4340,11 @@
> > > > > > >
> > > > > > > if (u->cleanup == NULL) {
> > > > > > > /* the request was already finalize
Re: Segfault when interpreting cached X-Accel-Redirect response
Hello!
On Tue, Feb 06, 2024 at 11:42:40AM +0100, Jan Prachař wrote:
> Hello Maxim,
>
> On Tue, 2024-02-06 at 00:46 +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Mon, Feb 05, 2024 at 06:01:54PM +0100, Jan Prachař wrote:
> >
> > > Hello,
> > >
> > > thank you for your responses.
> > >
> > > On Sat, 2024-02-03 at 04:25 +0300, Maxim Dounin wrote:
> > > > Hello!
> > > >
> > > > On Fri, Feb 02, 2024 at 01:47:51PM +0100, Jan Prachař wrote:
> > > >
> > > > > On Fri, 2024-02-02 at 12:48 +0100, Jiří Setnička via nginx-devel
> > > > > wrote:
> > > > > > Hello,
> > > > > >
> > > > > > Also, I believe that the core of the problem is because of the
> > > > > > ngx_http_finalize_request(r, NGX_DONE); call in the
> > > > > > ngx_http_upstream_process_headers function. This call is needed
> > > > > > when
> > > > > > doing an internal redirect after the real upstream request (to
> > > > > > close the
> > > > > > upstream request), but when serving from the cache, there is no
> > > > > > upstream
> > > > > > request to close and this call causes ngx_http_set_lingering_close
> > > > > > to be
> > > > > > called from the ngx_http_finalize_connection with no active request
> > > > > > on
> > > > > > the connection yielding to the segfault.
> > > > >
> > > > > Hello,
> > > > >
> > > > > I am Jiří's colleague, and so I have taken a closer look at the
> > > > > problem. Another
> > > > > indication of the issue is the alert in the error log for
> > > > > non-keepalive connections,
> > > > > stating "http request count is zero while closing request."
> > > > >
> > > > > Upon reviewing the nginx source code, I discovered that the function
> > > > > ngx_http_upstream_finalize_request(), when called with rc =
> > > > > NGX_DECLINED, does not invoke
> > > > > ngx_http_finalize_request(). However, when there is nothing to clean
> > > > > up (u->cleanup ==
> > > > > NULL), it does. Therefore, I believe the appropriate fix is to follow
> > > > > the patch below.
> > > > >
> > > > > Best, Jan Prachař
> > > > >
> > > > > # User Jan Prachař
> > > > > # Date 1706877176 -3600
> > > > > # Fri Feb 02 13:32:56 2024 +0100
> > > > > # Node ID 851c994b48c48c9cd3d32b9aa402f4821aeb8bb2
> > > > > # Parent cf3d537ec6706f8713a757df256f2cfccb8f9b01
> > > > > Upstream: Fix "request count is zero" when procesing X-Accel-Redirect
> > > > >
> > > > > ngx_http_upstream_finalize_request(r, u, NGX_DECLINED) should not call
> > > > > ngx_http_finalize_request().
> > > > >
> > > > > diff -r cf3d537ec670 -r 851c994b48c4 src/http/ngx_http_upstream.c
> > > > > --- a/src/http/ngx_http_upstream.c Thu Nov 26 21:00:25 2020 +0100
> > > > > +++ b/src/http/ngx_http_upstream.c Fri Feb 02 13:32:56 2024 +0100
> > > > > @@ -4340,6 +4340,11 @@
> > > > >
> > > > > if (u->cleanup == NULL) {
> > > > > /* the request was already finalized */
> > > > > +
> > > > > +if (rc == NGX_DECLINED) {
> > > > > +return;
> > > > > +}
> > > > > +
> > > > > ngx_http_finalize_request(r, NGX_DONE);
> > > > > return;
> > > > > }
> > > >
> > > > I somewhat agree: the approach suggested by Jiří certainly looks
> > > > incorrect. The ngx_http_upstream_cache_send() function, which
> > > > calls ngx_http_upstream_process_headers() with r->cached set, can
> > > > be used in two contexts: before the cleanup handler is installed
> > > > (i.e., when sending a cached response during upstream request
> > > > initialization) and after it is installed (i.e., when sending a
> > > > stale cached response on upstream errors). In the latter case
> > > > skipping finalization would mean a socket leak.
> > > >
> > > > Still, checking for NGX_DECLINED explic
Re: Segfault when interpreting cached X-Accel-Redirect response
Hello!
On Mon, Feb 05, 2024 at 06:01:54PM +0100, Jan Prachař wrote:
> Hello,
>
> thank you for your responses.
>
> On Sat, 2024-02-03 at 04:25 +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Fri, Feb 02, 2024 at 01:47:51PM +0100, Jan Prachař wrote:
> >
> > > On Fri, 2024-02-02 at 12:48 +0100, Jiří Setnička via nginx-devel wrote:
> > > > Hello,
> > > >
> > > > Also, I believe that the core of the problem is because of the
> > > > ngx_http_finalize_request(r, NGX_DONE); call in the
> > > > ngx_http_upstream_process_headers function. This call is needed when
> > > > doing an internal redirect after the real upstream request (to close
> > > > the
> > > > upstream request), but when serving from the cache, there is no
> > > > upstream
> > > > request to close and this call causes ngx_http_set_lingering_close to
> > > > be
> > > > called from the ngx_http_finalize_connection with no active request on
> > > > the connection yielding to the segfault.
> > >
> > > Hello,
> > >
> > > I am Jiří's colleague, and so I have taken a closer look at the problem.
> > > Another
> > > indication of the issue is the alert in the error log for non-keepalive
> > > connections,
> > > stating "http request count is zero while closing request."
> > >
> > > Upon reviewing the nginx source code, I discovered that the function
> > > ngx_http_upstream_finalize_request(), when called with rc = NGX_DECLINED,
> > > does not invoke
> > > ngx_http_finalize_request(). However, when there is nothing to clean up
> > > (u->cleanup ==
> > > NULL), it does. Therefore, I believe the appropriate fix is to follow the
> > > patch below.
> > >
> > > Best, Jan Prachař
> > >
> > > # User Jan Prachař
> > > # Date 1706877176 -3600
> > > # Fri Feb 02 13:32:56 2024 +0100
> > > # Node ID 851c994b48c48c9cd3d32b9aa402f4821aeb8bb2
> > > # Parent cf3d537ec6706f8713a757df256f2cfccb8f9b01
> > > Upstream: Fix "request count is zero" when procesing X-Accel-Redirect
> > >
> > > ngx_http_upstream_finalize_request(r, u, NGX_DECLINED) should not call
> > > ngx_http_finalize_request().
> > >
> > > diff -r cf3d537ec670 -r 851c994b48c4 src/http/ngx_http_upstream.c
> > > --- a/src/http/ngx_http_upstream.c Thu Nov 26 21:00:25 2020 +0100
> > > +++ b/src/http/ngx_http_upstream.c Fri Feb 02 13:32:56 2024 +0100
> > > @@ -4340,6 +4340,11 @@
> > >
> > > if (u->cleanup == NULL) {
> > > /* the request was already finalized */
> > > +
> > > +if (rc == NGX_DECLINED) {
> > > +return;
> > > +}
> > > +
> > > ngx_http_finalize_request(r, NGX_DONE);
> > > return;
> > > }
> >
> > I somewhat agree: the approach suggested by Jiří certainly looks
> > incorrect. The ngx_http_upstream_cache_send() function, which
> > calls ngx_http_upstream_process_headers() with r->cached set, can
> > be used in two contexts: before the cleanup handler is installed
> > (i.e., when sending a cached response during upstream request
> > initialization) and after it is installed (i.e., when sending a
> > stale cached response on upstream errors). In the latter case
> > skipping finalization would mean a socket leak.
> >
> > Still, checking for NGX_DECLINED explicitly also looks wrong, for
> > a number of reasons.
> >
> > First, the specific code path isn't just for "nothing to clean
> > up", it's for the very specific case when the request was already
> > finalized due to filter finalization, see 5994:5abf5af257a7. This
> > code path is not expected to be triggered when the cleanup handler
> > isn't installed yet - before the cleanup handler is installed,
> > upstream code is expected to call ngx_http_finalize_request()
> > directly instead. And it would be semantically wrong to check for
> > NGX_DECLINED: if it's here, it means something already gone wrong.
> >
> > I think the generic issue here is that
> > ngx_http_upstream_process_headers(), which is normally used for
> > upstream responses and calls ngx_http_upstream_finalize_request(),
> > is also used for cached responses. Still, it assumes it is used
> > for an upstream response, and calls
> > ngx_http_upstream_finalize_reques
Re: Segfault when interpreting cached X-Accel-Redirect response
ERROR in other places to trigger
appropriate error handling instead of calling
ngx_http_upstream_finalize_request() directly (this no longer
tries to return 500 Internal Server Error response though, as
doing so might be unsafe after copying some of the cached headers
to the response).
Please take a look if it works for you.
diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -588,10 +588,6 @@ ngx_http_upstream_init_request(ngx_http_
if (rc == NGX_OK) {
rc = ngx_http_upstream_cache_send(r, u);
-if (rc == NGX_DONE) {
-return;
-}
-
if (rc == NGX_HTTP_UPSTREAM_INVALID_HEADER) {
rc = NGX_DECLINED;
r->cached = 0;
@@ -1088,7 +1084,7 @@ ngx_http_upstream_cache_send(ngx_http_re
if (rc == NGX_OK) {
if (ngx_http_upstream_process_headers(r, u) != NGX_OK) {
-return NGX_DONE;
+return NGX_ERROR;
}
return ngx_http_cache_send(r);
@@ -2516,7 +2512,14 @@ ngx_http_upstream_process_header(ngx_htt
}
}
-if (ngx_http_upstream_process_headers(r, u) != NGX_OK) {
+rc = ngx_http_upstream_process_headers(r, u);
+
+if (rc == NGX_DONE) {
+return;
+}
+
+if (rc == NGX_ERROR) {
+ngx_http_upstream_finalize_request(r, u, NGX_ERROR);
return;
}
@@ -2576,10 +2579,6 @@ ngx_http_upstream_test_next(ngx_http_req
u->cache_status = NGX_HTTP_CACHE_STALE;
rc = ngx_http_upstream_cache_send(r, u);
-if (rc == NGX_DONE) {
-return NGX_OK;
-}
-
if (rc == NGX_HTTP_UPSTREAM_INVALID_HEADER) {
rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
}
@@ -2621,10 +2620,6 @@ ngx_http_upstream_test_next(ngx_http_req
u->cache_status = NGX_HTTP_CACHE_REVALIDATED;
rc = ngx_http_upstream_cache_send(r, u);
-if (rc == NGX_DONE) {
-return NGX_OK;
-}
-
if (rc == NGX_HTTP_UPSTREAM_INVALID_HEADER) {
rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
}
@@ -2827,7 +2822,8 @@ ngx_http_upstream_process_headers(ngx_ht
}
if (u->headers_in.x_accel_redirect
-&& !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))
+&& !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT)
+&& !r->cached)
{
ngx_http_upstream_finalize_request(r, u, NGX_DECLINED);
@@ -2918,18 +2914,14 @@ ngx_http_upstream_process_headers(ngx_ht
if (hh) {
if (hh->copy_handler(r, [i], hh->conf) != NGX_OK) {
-ngx_http_upstream_finalize_request(r, u,
- NGX_HTTP_INTERNAL_SERVER_ERROR);
-return NGX_DONE;
+return NGX_ERROR;
}
continue;
}
if (ngx_http_upstream_copy_header_line(r, [i], 0) != NGX_OK) {
-ngx_http_upstream_finalize_request(r, u,
- NGX_HTTP_INTERNAL_SERVER_ERROR);
-return NGX_DONE;
+return NGX_ERROR;
}
}
@@ -4442,10 +4434,6 @@ ngx_http_upstream_next(ngx_http_request_
u->cache_status = NGX_HTTP_CACHE_STALE;
rc = ngx_http_upstream_cache_send(r, u);
-if (rc == NGX_DONE) {
-return;
-}
-
if (rc == NGX_HTTP_UPSTREAM_INVALID_HEADER) {
rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
}
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] SSL: fixed $ssl_curves allocation error handling
Hello!
On Fri, Jan 26, 2024 at 02:36:29PM +0400, Sergey Kandaurov wrote:
> # HG changeset patch
> # User Sergey Kandaurov
> # Date 1706265240 -14400
> # Fri Jan 26 14:34:00 2024 +0400
> # Node ID 2f70dd17c16461f833eafec2dcf9193557bfb176
> # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07
> SSL: fixed $ssl_curves allocation error handling.
>
> diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c
> +++ b/src/event/ngx_event_openssl.c
> @@ -5187,6 +5187,9 @@ ngx_ssl_get_curves(ngx_connection_t *c,
> }
>
> curves = ngx_palloc(pool, n * sizeof(int));
> +if (curves == NULL) {
> +return NGX_ERROR;
> +}
>
> n = SSL_get1_curves(c->ssl->connection, curves);
> len = 0;
Looks good.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 2 of 4] Upstream: fixed usage of closed sockets with filter finalization
Hello!
On Mon, Jan 29, 2024 at 05:21:43PM +0400, Sergey Kandaurov wrote:
>
> > On 29 Jan 2024, at 10:43, Maxim Dounin wrote:
> >
> > Hello!
> >
> > On Fri, Jan 26, 2024 at 04:26:00PM +0400, Sergey Kandaurov wrote:
> >
> >>> On 27 Nov 2023, at 06:50, Maxim Dounin wrote:
> >>>
> >>> # HG changeset patch
> >>> # User Maxim Dounin
> >>> # Date 1701049758 -10800
> >>> # Mon Nov 27 04:49:18 2023 +0300
> >>> # Node ID faf0b9defc76b8683af466f8a950c2c241382970
> >>> # Parent a5e39e9d1f4c84dcbe6a2f9e079372a3d63aef0b
> >>> Upstream: fixed usage of closed sockets with filter finalization.
> >>>
> >>> When filter finalization is triggered when working with an upstream
> >>> server,
> >>> and error_page redirects request processing to some simple handler,
> >>> ngx_http_request_finalize() triggers request termination when the response
> >>> is sent. In particular, via the upstream cleanup handler, nginx will
> >>> close
> >>> the upstream connection and the corresponding socket.
> >>>
> >>> Still, this can happen to be with ngx_event_pipe() on stack. While
> >>> the code will set p->downstream_error due to NGX_ERROR returned from the
> >>> output filter chain by filter finalization, otherwise the error will be
> >>> ignored till control returns to ngx_http_upstream_process_request().
> >>> And event pipe might try reading from the (already closed) socket,
> >>> resulting
> >>> in "readv() failed (9: Bad file descriptor) while reading upstream" errors
> >>> (or even segfaults with SSL).
> >>>
> >>> Such errors were seen with the following configuration:
> >>>
> >>> location /t2 {
> >>> proxy_pass http://127.0.0.1:8080/big;
> >>>
> >>> image_filter_buffer 10m;
> >>> image_filter resize 150 100;
> >>> error_page 415 = /empty;
> >>> }
> >>>
> >>> location /empty {
> >>> return 204;
> >>> }
> >>>
> >>> location /big {
> >>> # big enough static file
> >>> }
> >>>
> >>> Fix is to set p->upstream_error in ngx_http_upstream_finalize_request(),
> >>> so the existing checks in ngx_event_pipe_read_upstream() will prevent
> >>> further reading from the closed upstream connection.
> >>>
> >>> Similarly, p->upstream_error is now checked when handling events at
> >>> ngx_event_pipe() exit, as checking p->upstream->fd is not enough if
> >>> keepalive upstream connections are being used and the connection was
> >>> saved to cache during request termination.
> >>>
> >>
> >> Setting p->upstream_error in ngx_http_upstream_finalize_request()
> >> may look suspicious, because it is used to be set on connection errors
> >> such as upstream timeout or recv error, or, as a recently introduced
> >> exception in the fastcgi module, - also when the FastCGI record ends
> >> prematurely, before receiving all the expected content.
> >> But technically I think this is quite correct, because we no longer
> >> want to receive further data, and also (and you mention this in the
> >> commit log) this repeats closing an upstream connection socket in
> >> the same place in ngx_http_upstream_finalize_request().
> >> So I think it should be fine.
> >
> > The biggest concern I personally see here is with the added
> > p->upstream_error check at ngx_event_pipe() exit. If there is a
> > real upstream error, such as when the connection is reset by the
> > upstream server, and if we want the pipe to be active for some
> > time (for example, if we want it to continue writing to the
> > downstream connection), there will be no ngx_handle_read_event()
> > call. For level-triggered event methods this means that the read
> > event for the upstream connection will be generated again and
> > again.
> >
> > This shouldn't be the problem for existing ngx_event_pipe() uses
> > though, as p->upstream_error is anyway triggers
> > ngx_http_upstream_finalize_request().
> >
> > Still, we can consider introducing a separate flag, such as
> > p->upstream_closed, or clearing p->upstream, and checking these in
> > ngx_event_pipe() instead. This probably
Re: [PATCH 1 of 4] Fixed request termination with AIO and subrequests (ticket #2555)
Hello!
On Mon, Jan 29, 2024 at 06:07:58PM +0400, Roman Arutyunyan wrote:
> Hi,
>
> On Mon, Jan 29, 2024 at 10:58:09AM +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Fri, Jan 26, 2024 at 04:02:30PM +0400, Roman Arutyunyan wrote:
> >
> > > On Mon, Nov 27, 2023 at 05:50:24AM +0300, Maxim Dounin wrote:
> > > > # HG changeset patch
> > > > # User Maxim Dounin
> > > > # Date 1701049682 -10800
> > > > # Mon Nov 27 04:48:02 2023 +0300
> > > > # Node ID a5e39e9d1f4c84dcbe6a2f9e079372a3d63aef0b
> > > > # Parent f366007dd23a6ce8e8427c1b3042781b618a2ade
> > > > Fixed request termination with AIO and subrequests (ticket #2555).
> > > >
> > > > When a request was terminated due to an error via
> > > > ngx_http_terminate_request()
> > > > while an AIO operation was running in a subrequest, various issues were
> > > > observed. This happened because ngx_http_request_finalizer() was only
> > > > set
> > > > in the subrequest where ngx_http_terminate_request() was called, but not
> > > > in the subrequest where the AIO operation was running. After completion
> > > > of the AIO operation resumed normal processing of the subrequest,
> > > > leading
> > > > to issues.
> > >
> > > Something is wrong with the last sentence.
> >
> > Thanks, rewritten as:
> >
> > ... After completion
> > of the AIO operation normal processing of the subrequest was resumed,
> > leading
> > to issues.
> >
> > > > In particular, in case of the upstream module, termination of the
> > > > request
> > > > called upstream cleanup, which closed the upstream connection.
> > > > Attempts to
> > > > further work with the upstream connection after AIO operation completion
> > > > resulted in segfaults in ngx_ssl_recv(), "readv() failed (9: Bad file
> > > > descriptor) while reading upstream" errors, or socket leaks.
> > >
> > > Can you elaborate on socket leaks?
> >
> > For example, consider a request which is waiting for additional
> > data from the upstream, so the only timer is the read timer for
> > the upstream connection, and which is terminated because the
> > client closed the connection. Request termination will remove the
> > only timer. Still, the client connection is not yet closed by
> > nginx. So as long as the request is not actually freed following
> > completion of the AIO operation, this is a socket leak: we have no
> > timers left, and no further events expected. And this can easily
> > happen if neither segfault nor readv() error was triggered (for
> > example, if p->upstream->read->ready was not set during AIO
> > operation completion).
> >
> > > > In ticket #2555, issues were observed with the following configuration
> > > > with cache background update (with thread writing instrumented to
> > > > introduce a delay, when a client closes the connection during an
> > > > update):
> > > >
> > > > location = /background-and-aio-write {
> > > > proxy_pass ...
> > > > proxy_cache one;
> > > > proxy_cache_valid 200 1s;
> > > > proxy_cache_background_update on;
> > > > proxy_cache_use_stale updating;
> > > > aio threads;
> > > > aio_write on;
> > > > limit_rate 1000;
> > > > }
> > > >
> > > > Similarly, the same issue can be seen with SSI, and can be caused by
> > > > errors in subrequests, such as in the following configuration
> > > > (were "/proxy" uses AIO, and "/sleep" returns 444 after some delay,
> > >
> > > s/were/where/ ?
> >
> > Fixed, thanks.
> >
> > > > causing request termination):
> > > >
> > > > location = /ssi-active-boom {
> > > > ssi on;
> > > > ssi_types *;
> > > > return 200 '
> > > >
> > > >
> > > >';
> > > > limit_rate 1000;
> > > > }
> > > >
> > > > Or the same with both AIO operation and the error in non-active
> > > > subrequests
> > > > (which needs slightly different handling, see below):
> > > >
> > &g
Re: nginx-tests SSL tests failing out of the box?
Hello!
On Mon, Jan 29, 2024 at 05:23:15PM +0400, Sergey Kandaurov wrote:
> > On 29 Jan 2024, at 07:24, Maxim Dounin wrote:
> >
> > Hello!
> >
> > On Sat, Jan 27, 2024 at 07:19:45AM +0300, Maxim Dounin wrote:
> >
> >> Hello!
> >>
> >> On Fri, Jan 26, 2024 at 09:29:58PM +0400, Sergey Kandaurov wrote:
> >>
> >>> On Thu, Jan 25, 2024 at 11:38:57PM +0300, Maxim Dounin wrote:
> >>>> Hello!
> >>>>
> >>>> On Thu, Jan 25, 2024 at 06:59:36PM +, Mayerhofer, Austin via
> >>>> nginx-devel wrote:
> >>>>
> >>>>> Hi all,
> >>>>>
> >>>>> I have not made any changes to NGINX. Vanilla NGINX (./configure with
> >>>>> no flags) passes all tests that run, but when compiling with SSL, not
> >>>>> all SSL tests are passing. Is this expected, or do I need to configure
> >>>>> nginx further aside from adding the --with-http_ssl_module flag? Do
> >>>>> each of the failing tests below require separate fixes, or is there a
> >>>>> one-size-fits-all solution for all of them?
> >>>>>
> >>>>> OS: MacOS 12.6.3
> >>>>> Chip: Apple M1 Max
> >>>>> NGINX: 1.24.0 built from source code with ./configure --with-debug
> >>>>> --with-http_ssl_module
> >>>>> Nginx-tests:
> >>>>> https://github.com/nginx/nginx-tests/tree/4c2ad8093952706f327d04887c5546bad91b75a6
> >>>>> OpenSSL: 3.2.0 (/opt/homebrew/bin/openssl)
> >>>>> Perl: 5.30.3 (/usr/bin/perl)
> >>>>>
> >>>>> When I run
> >>>>>
> >>>>> ```
> >>>>> TEST_NGINX_BINARY=/usr/local/nginx/sbin/nginx prove -v ssl.t
> >>>>> ```
> >>>>>
> >>>>> I see
> >>>>>
> >>>>> ```
> >>>>> not ok 2 - session reused
> >>>>>
> >>>>> # Failed test 'session reused'
> >>>>> # at ssl.t line 187.
> >>>>> # 'HTTP/1.1 200 OK
> >>>>> # Server: nginx/1.24.0
> >>>>> # Date: Thu, 25 Jan 2024 18:50:10 GMT
> >>>>> # Content-Type: text/plain
> >>>>> # Content-Length: 6
> >>>>> # Connection: close
> >>>>> #
> >>>>> # body .'
> >>>>> # doesn't match '(?^m:^body r$)'
> >>>>> ```
> >>>>
> >>>> [...]
> >>>>
> >>>> It looks like SSL session reuse is broken in Perl you are
> >>>> using. This might be the case if, for example, Net::SSLeay in
> >>>> your installation was compiled with system LibreSSL as an SSL
> >>>> library - at least on the server side LibreSSL simply does not
> >>>> support session reuse with TLSv1.3.
> >>>>
> >>>> Test suite checks if nginx was compiled with LibreSSL and marks
> >>>> appropriate tests as TODO, but if the Perl module is broken
> >>>> instead, the test will fail.
> >>>>
> >>>
> >>> Well, technically, we could test this and skip appropriately:
> >>>
> >>> diff --git a/ssl_session_reuse.t b/ssl_session_reuse.t
> >>> --- a/ssl_session_reuse.t
> >>> +++ b/ssl_session_reuse.t
> >>> @@ -166,7 +166,9 @@ local $TODO = 'no TLSv1.3 sessions, old
> >>> local $TODO = 'no TLSv1.3 sessions, old IO::Socket::SSL'
> >>> if $IO::Socket::SSL::VERSION < 2.061 && test_tls13();
> >>> local $TODO = 'no TLSv1.3 sessions in LibreSSL'
> >>> - if $t->has_module('LibreSSL') && test_tls13();
> >>> + if ($t->has_module('LibreSSL')
> >>> + || Net::SSLeay::constant("LIBRESSL_VERSION_NUMBER"))
> >>> + && test_tls13();
> >>>
> >>> is(test_reuse(8443), 1, 'tickets reused');
> >>> is(test_reuse(8444), 1, 'tickets and cache reused');
> >>>
> >>> But I see little to no purpose: if the testing tool is broken
> >>> in various unexpected ways (another example is X509_V_ERR_INVALID_PURPOSE
> >>> in peer certificate verification as reported in the adjacent thread),
> >>> I think we barely can handle this in general.
> >>
> >> I generally agree.
> >>
> >> Stil
Re: Nginx-tests stream_ssl_conf_command.t test hanging indefinitely
Hello! On Fri, Jan 26, 2024 at 01:14:56AM +, Mayerhofer, Austin via nginx-devel wrote: > Hey Maxim, > > Thanks, I installed homebrew’s Perl and all these tests are > passing now, woohoo! > > However a few others are failing now including ssl_ocsp.t and > ssl_verify_depth.t, failing 13/17 and 3/11 tests respectively > with the same error: > > ``` > # Failed test 'verify depth 2 - end' > # at ssl_verify_depth.t line 169. > # 'HTTP/1.1 400 Bad Request > # Server: nginx/1.24.0 > # Date: Fri, 26 Jan 2024 01:08:10 GMT > # Content-Type: text/html > # Content-Length: 215 > # Connection: close > # X-Client: CN=end > # X-Verify: FAILED:unsuitable certificate purpose [...] Should be fixed with this patch: https://mailman.nginx.org/pipermail/nginx-devel/2024-January/TSFBB5DWWQKXKDTGVLSH5VWJYMRCMCV4.html Thanks for reporting this. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 4] Fixed request termination with AIO and subrequests (ticket #2555)
Hello!
On Fri, Jan 26, 2024 at 04:02:30PM +0400, Roman Arutyunyan wrote:
> On Mon, Nov 27, 2023 at 05:50:24AM +0300, Maxim Dounin wrote:
> > # HG changeset patch
> > # User Maxim Dounin
> > # Date 1701049682 -10800
> > # Mon Nov 27 04:48:02 2023 +0300
> > # Node ID a5e39e9d1f4c84dcbe6a2f9e079372a3d63aef0b
> > # Parent f366007dd23a6ce8e8427c1b3042781b618a2ade
> > Fixed request termination with AIO and subrequests (ticket #2555).
> >
> > When a request was terminated due to an error via
> > ngx_http_terminate_request()
> > while an AIO operation was running in a subrequest, various issues were
> > observed. This happened because ngx_http_request_finalizer() was only set
> > in the subrequest where ngx_http_terminate_request() was called, but not
> > in the subrequest where the AIO operation was running. After completion
> > of the AIO operation resumed normal processing of the subrequest, leading
> > to issues.
>
> Something is wrong with the last sentence.
Thanks, rewritten as:
... After completion
of the AIO operation normal processing of the subrequest was resumed, leading
to issues.
> > In particular, in case of the upstream module, termination of the request
> > called upstream cleanup, which closed the upstream connection. Attempts to
> > further work with the upstream connection after AIO operation completion
> > resulted in segfaults in ngx_ssl_recv(), "readv() failed (9: Bad file
> > descriptor) while reading upstream" errors, or socket leaks.
>
> Can you elaborate on socket leaks?
For example, consider a request which is waiting for additional
data from the upstream, so the only timer is the read timer for
the upstream connection, and which is terminated because the
client closed the connection. Request termination will remove the
only timer. Still, the client connection is not yet closed by
nginx. So as long as the request is not actually freed following
completion of the AIO operation, this is a socket leak: we have no
timers left, and no further events expected. And this can easily
happen if neither segfault nor readv() error was triggered (for
example, if p->upstream->read->ready was not set during AIO
operation completion).
> > In ticket #2555, issues were observed with the following configuration
> > with cache background update (with thread writing instrumented to
> > introduce a delay, when a client closes the connection during an update):
> >
> > location = /background-and-aio-write {
> > proxy_pass ...
> > proxy_cache one;
> > proxy_cache_valid 200 1s;
> > proxy_cache_background_update on;
> > proxy_cache_use_stale updating;
> > aio threads;
> > aio_write on;
> > limit_rate 1000;
> > }
> >
> > Similarly, the same issue can be seen with SSI, and can be caused by
> > errors in subrequests, such as in the following configuration
> > (were "/proxy" uses AIO, and "/sleep" returns 444 after some delay,
>
> s/were/where/ ?
Fixed, thanks.
> > causing request termination):
> >
> > location = /ssi-active-boom {
> > ssi on;
> > ssi_types *;
> > return 200 '
> >
> >
> >';
> > limit_rate 1000;
> > }
> >
> > Or the same with both AIO operation and the error in non-active subrequests
> > (which needs slightly different handling, see below):
> >
> > location = /ssi-non-active-boom {
> > ssi on;
> > ssi_types *;
> > return 200 '
> >
> >
> >
> >';
> > limit_rate 1000;
> > }
> >
> > Similarly, issues can be observed with just static files. However,
> > with static files potential impact is limited due to timeout safeguards
> > in ngx_http_writer(), and the fact that c->error is set during request
> > termination.
> > In a simple configuration with an AIO operation in the active subrequest,
> > such as in the following configuration, the connection is closed right
> > after completion of the AIO operation anyway, since ngx_http_writer()
> > tries to write to the connection and fails due to c->error set:
> >
> > location = /ssi-active-static-boom {
> > ssi on;
> > ssi_types *;
> > return 200 '
> >
> >
> >';
> > limit_rate 1000;
>
Re: [PATCH 4 of 4] AIO operations now add timers (ticket #2162)
Hello!
On Fri, Jan 26, 2024 at 04:27:30PM +0400, Sergey Kandaurov wrote:
>
> > On 27 Nov 2023, at 06:50, Maxim Dounin wrote:
> >
> > # HG changeset patch
> > # User Maxim Dounin
> > # Date 1701050170 -10800
> > # Mon Nov 27 04:56:10 2023 +0300
> > # Node ID 00c3e7333145ddb5ea0eeaaa66b3d9c26973c9c2
> > # Parent 61d08e4cf97cc073200ec32fc6ada9a2d48ffe51
> > AIO operations now add timers (ticket #2162).
> >
> > Each AIO (thread IO) operation being run is now accompanied with 1-minute
> > timer. This timer prevents unexpected shutdown of the worker process while
> > an AIO operation is running, and logs an alert if the operation is running
> > for too long.
> >
> > This fixes "open socket left" alerts during worker processes shutdown
> > due to pending AIO (or thread IO) operations while corresponding requests
> > have no timers. In particular, such errors were observed while reading
> > cache headers (ticket #2162), and with worker_shutdown_timeout.
> >
> > diff --git a/src/http/ngx_http_copy_filter_module.c
> > b/src/http/ngx_http_copy_filter_module.c
> > --- a/src/http/ngx_http_copy_filter_module.c
> > +++ b/src/http/ngx_http_copy_filter_module.c
> > @@ -170,6 +170,8 @@ ngx_http_copy_aio_handler(ngx_output_cha
> > file->aio->data = r;
> > file->aio->handler = ngx_http_copy_aio_event_handler;
> >
> > +ngx_add_timer(>aio->event, 6);
> > +
> > r->main->blocked++;
> > r->aio = 1;
> > ctx->aio = 1;
> > @@ -192,6 +194,17 @@ ngx_http_copy_aio_event_handler(ngx_even
> > ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
> >"http aio: \"%V?%V\"", >uri, >args);
> >
> > +if (ev->timedout) {
> > +ngx_log_error(NGX_LOG_ALERT, c->log, 0,
> > + "aio operation took too long");
> > +ev->timedout = 0;
> > +return;
> > +}
> > +
> > +if (ev->timer_set) {
> > +ngx_del_timer(ev);
> > +}
> > +
> > r->main->blocked--;
> > r->aio = 0;
> >
> > @@ -273,6 +286,8 @@ ngx_http_copy_thread_handler(ngx_thread_
> > return NGX_ERROR;
> > }
> >
> > +ngx_add_timer(>event, 6);
> > +
> > r->main->blocked++;
> > r->aio = 1;
> >
> > @@ -297,6 +312,17 @@ ngx_http_copy_thread_event_handler(ngx_e
> > ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
> >"http thread: \"%V?%V\"", >uri, >args);
> >
> > +if (ev->timedout) {
> > +ngx_log_error(NGX_LOG_ALERT, c->log, 0,
> > + "thread operation took too long");
> > +ev->timedout = 0;
> > +return;
> > +}
> > +
> > +if (ev->timer_set) {
> > +ngx_del_timer(ev);
> > +}
> > +
> > r->main->blocked--;
> > r->aio = 0;
> >
> > diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
> > --- a/src/http/ngx_http_file_cache.c
> > +++ b/src/http/ngx_http_file_cache.c
> > @@ -705,6 +705,8 @@ ngx_http_file_cache_aio_read(ngx_http_re
> > c->file.aio->data = r;
> > c->file.aio->handler = ngx_http_cache_aio_event_handler;
> >
> > +ngx_add_timer(>file.aio->event, 6);
> > +
> > r->main->blocked++;
> > r->aio = 1;
> >
> > @@ -752,6 +754,17 @@ ngx_http_cache_aio_event_handler(ngx_eve
> > ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
> >"http file cache aio: \"%V?%V\"", >uri, >args);
> >
> > +if (ev->timedout) {
> > +ngx_log_error(NGX_LOG_ALERT, c->log, 0,
> > + "aio operation took too long");
> > +ev->timedout = 0;
> > +return;
> > +}
> > +
> > +if (ev->timer_set) {
> > +ngx_del_timer(ev);
> > +}
> > +
> > r->main->blocked--;
> > r->aio = 0;
> >
> > @@ -810,6 +823,8 @@ ngx_http_cache_thread_handler(ngx_thread
> > return NGX_ERROR;
> > }
> >
> > +ngx_add_timer(>event, 6);
> > +
> > r->main->blocked++;
> > r->aio = 1;
> >
> > @@ -831,6 +846,17 @@ ngx_http_cache_thread_event_handler(ngx_
> > ngx
Re: [PATCH 3 of 4] Silenced complaints about socket leaks on forced termination
Hello!
On Fri, Jan 26, 2024 at 04:26:21PM +0400, Sergey Kandaurov wrote:
>
> > On 27 Nov 2023, at 06:50, Maxim Dounin wrote:
> >
> > # HG changeset patch
> > # User Maxim Dounin
> > # Date 1701049787 -10800
> > # Mon Nov 27 04:49:47 2023 +0300
> > # Node ID 61d08e4cf97cc073200ec32fc6ada9a2d48ffe51
> > # Parent faf0b9defc76b8683af466f8a950c2c241382970
> > Silenced complaints about socket leaks on forced termination.
> >
> > When graceful shutdown was requested, and then nginx was forced to
> > do fast shutdown, it used to (incorrectly) complain about open sockets
> > left in connections which weren't yet closed when fast shutdown
> > was requested.
> >
> > Fix is to avoid complaining about open sockets when fast shutdown was
> > requested after graceful one. Abnormal termination, if requested with
> > the WINCH signal, can still happen though.
>
> I've been wondering about such IMHO odd behaviour and support the fix.
> There might be an opinion that once you requested graceful shutdown,
> you have to wait until it's done, but I think that requesting fast
> shutdown afterwards should be legitimate.
I tend to think that the existing behaviour might be usable in some
situations, like when one wants to look into remaining connections
after waiting for some time for graceful shutdown to complete.
Still, it is very confusing for unaware people, and I've seen lots
of reports about socket leaks which in fact aren't. And, more
importantly, with the existing behaviour when looking at a socket
leak report you never know if it's real or not. So it is
certainly worth fixing.
>
> >
> > diff --git a/src/os/unix/ngx_process_cycle.c
> > b/src/os/unix/ngx_process_cycle.c
> > --- a/src/os/unix/ngx_process_cycle.c
> > +++ b/src/os/unix/ngx_process_cycle.c
> > @@ -948,7 +948,7 @@ ngx_worker_process_exit(ngx_cycle_t *cyc
> > }
> > }
> >
> > -if (ngx_exiting) {
> > +if (ngx_exiting && !ngx_terminate) {
> > c = cycle->connections;
> > for (i = 0; i < cycle->connection_n; i++) {
> > if (c[i].fd != -1
> > @@ -963,11 +963,11 @@ ngx_worker_process_exit(ngx_cycle_t *cyc
> > ngx_debug_quit = 1;
> > }
> > }
> > +}
> >
> > -if (ngx_debug_quit) {
> > -ngx_log_error(NGX_LOG_ALERT, cycle->log, 0, "aborting");
> > -ngx_debug_point();
> > -}
> > +if (ngx_debug_quit) {
> > +ngx_log_error(NGX_LOG_ALERT, cycle->log, 0, "aborting");
> > +ngx_debug_point();
> > }
> >
> > /*
> > diff --git a/src/os/win32/ngx_process_cycle.c
> > b/src/os/win32/ngx_process_cycle.c
> > --- a/src/os/win32/ngx_process_cycle.c
> > +++ b/src/os/win32/ngx_process_cycle.c
> > @@ -834,7 +834,7 @@ ngx_worker_process_exit(ngx_cycle_t *cyc
> > }
> > }
> >
> > -if (ngx_exiting) {
> > +if (ngx_exiting && !ngx_terminate) {
> > c = cycle->connections;
> > for (i = 0; i < cycle->connection_n; i++) {
> > if (c[i].fd != (ngx_socket_t) -1
>
> I think it's fine.
Thanks for looking, pushed to http://mdounin.ru/hg/nginx/.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 2 of 4] Upstream: fixed usage of closed sockets with filter finalization
Hello!
On Fri, Jan 26, 2024 at 04:26:00PM +0400, Sergey Kandaurov wrote:
> > On 27 Nov 2023, at 06:50, Maxim Dounin wrote:
> >
> > # HG changeset patch
> > # User Maxim Dounin
> > # Date 1701049758 -10800
> > # Mon Nov 27 04:49:18 2023 +0300
> > # Node ID faf0b9defc76b8683af466f8a950c2c241382970
> > # Parent a5e39e9d1f4c84dcbe6a2f9e079372a3d63aef0b
> > Upstream: fixed usage of closed sockets with filter finalization.
> >
> > When filter finalization is triggered when working with an upstream server,
> > and error_page redirects request processing to some simple handler,
> > ngx_http_request_finalize() triggers request termination when the response
> > is sent. In particular, via the upstream cleanup handler, nginx will close
> > the upstream connection and the corresponding socket.
> >
> > Still, this can happen to be with ngx_event_pipe() on stack. While
> > the code will set p->downstream_error due to NGX_ERROR returned from the
> > output filter chain by filter finalization, otherwise the error will be
> > ignored till control returns to ngx_http_upstream_process_request().
> > And event pipe might try reading from the (already closed) socket, resulting
> > in "readv() failed (9: Bad file descriptor) while reading upstream" errors
> > (or even segfaults with SSL).
> >
> > Such errors were seen with the following configuration:
> >
> >location /t2 {
> >proxy_pass http://127.0.0.1:8080/big;
> >
> >image_filter_buffer 10m;
> >image_filter resize 150 100;
> >error_page 415 = /empty;
> >}
> >
> >location /empty {
> >return 204;
> >}
> >
> >location /big {
> ># big enough static file
> >}
> >
> > Fix is to set p->upstream_error in ngx_http_upstream_finalize_request(),
> > so the existing checks in ngx_event_pipe_read_upstream() will prevent
> > further reading from the closed upstream connection.
> >
> > Similarly, p->upstream_error is now checked when handling events at
> > ngx_event_pipe() exit, as checking p->upstream->fd is not enough if
> > keepalive upstream connections are being used and the connection was
> > saved to cache during request termination.
> >
>
> Setting p->upstream_error in ngx_http_upstream_finalize_request()
> may look suspicious, because it is used to be set on connection errors
> such as upstream timeout or recv error, or, as a recently introduced
> exception in the fastcgi module, - also when the FastCGI record ends
> prematurely, before receiving all the expected content.
> But technically I think this is quite correct, because we no longer
> want to receive further data, and also (and you mention this in the
> commit log) this repeats closing an upstream connection socket in
> the same place in ngx_http_upstream_finalize_request().
> So I think it should be fine.
The biggest concern I personally see here is with the added
p->upstream_error check at ngx_event_pipe() exit. If there is a
real upstream error, such as when the connection is reset by the
upstream server, and if we want the pipe to be active for some
time (for example, if we want it to continue writing to the
downstream connection), there will be no ngx_handle_read_event()
call. For level-triggered event methods this means that the read
event for the upstream connection will be generated again and
again.
This shouldn't be the problem for existing ngx_event_pipe() uses
though, as p->upstream_error is anyway triggers
ngx_http_upstream_finalize_request().
Still, we can consider introducing a separate flag, such as
p->upstream_closed, or clearing p->upstream, and checking these in
ngx_event_pipe() instead. This probably would be a more clear
solution.
Updated patch below:
# HG changeset patch
# User Maxim Dounin
# Date 1706510064 -10800
# Mon Jan 29 09:34:24 2024 +0300
# Node ID 4a91a03dcd8df0652884ed6ebe9f7437ce82fd26
# Parent 7b630f6487068f7cc9dd83762fb4ea39f2f340e9
Upstream: fixed usage of closed sockets with filter finalization.
When filter finalization is triggered when working with an upstream server,
and error_page redirects request processing to some simple handler,
ngx_http_request_finalize() triggers request termination when the response
is sent. In particular, via the upstream cleanup handler, nginx will close
the upstream connection and the corresponding socket.
Still, this can happen to be with ngx_event_pipe() on stack. While
the code will set p->downstream_error due to NGX_ERROR returned from the
output filter chain by filter finalization, otherwise the error will be
ignored till control ret
Re: nginx-tests SSL tests failing out of the box?
Hello!
On Sat, Jan 27, 2024 at 07:19:45AM +0300, Maxim Dounin wrote:
> Hello!
>
> On Fri, Jan 26, 2024 at 09:29:58PM +0400, Sergey Kandaurov wrote:
>
> > On Thu, Jan 25, 2024 at 11:38:57PM +0300, Maxim Dounin wrote:
> > > Hello!
> > >
> > > On Thu, Jan 25, 2024 at 06:59:36PM +, Mayerhofer, Austin via
> > > nginx-devel wrote:
> > >
> > > > Hi all,
> > > >
> > > > I have not made any changes to NGINX. Vanilla NGINX (./configure with
> > > > no flags) passes all tests that run, but when compiling with SSL, not
> > > > all SSL tests are passing. Is this expected, or do I need to configure
> > > > nginx further aside from adding the --with-http_ssl_module flag? Do
> > > > each of the failing tests below require separate fixes, or is there a
> > > > one-size-fits-all solution for all of them?
> > > >
> > > > OS: MacOS 12.6.3
> > > > Chip: Apple M1 Max
> > > > NGINX: 1.24.0 built from source code with ./configure --with-debug
> > > > --with-http_ssl_module
> > > > Nginx-tests:
> > > > https://github.com/nginx/nginx-tests/tree/4c2ad8093952706f327d04887c5546bad91b75a6
> > > > OpenSSL: 3.2.0 (/opt/homebrew/bin/openssl)
> > > > Perl: 5.30.3 (/usr/bin/perl)
> > > >
> > > > When I run
> > > >
> > > > ```
> > > > TEST_NGINX_BINARY=/usr/local/nginx/sbin/nginx prove -v ssl.t
> > > > ```
> > > >
> > > > I see
> > > >
> > > > ```
> > > > not ok 2 - session reused
> > > >
> > > > # Failed test 'session reused'
> > > > # at ssl.t line 187.
> > > > # 'HTTP/1.1 200 OK
> > > > # Server: nginx/1.24.0
> > > > # Date: Thu, 25 Jan 2024 18:50:10 GMT
> > > > # Content-Type: text/plain
> > > > # Content-Length: 6
> > > > # Connection: close
> > > > #
> > > > # body .'
> > > > # doesn't match '(?^m:^body r$)'
> > > > ```
> > >
> > > [...]
> > >
> > > It looks like SSL session reuse is broken in Perl you are
> > > using. This might be the case if, for example, Net::SSLeay in
> > > your installation was compiled with system LibreSSL as an SSL
> > > library - at least on the server side LibreSSL simply does not
> > > support session reuse with TLSv1.3.
> > >
> > > Test suite checks if nginx was compiled with LibreSSL and marks
> > > appropriate tests as TODO, but if the Perl module is broken
> > > instead, the test will fail.
> > >
> >
> > Well, technically, we could test this and skip appropriately:
> >
> > diff --git a/ssl_session_reuse.t b/ssl_session_reuse.t
> > --- a/ssl_session_reuse.t
> > +++ b/ssl_session_reuse.t
> > @@ -166,7 +166,9 @@ local $TODO = 'no TLSv1.3 sessions, old
> > local $TODO = 'no TLSv1.3 sessions, old IO::Socket::SSL'
> > if $IO::Socket::SSL::VERSION < 2.061 && test_tls13();
> > local $TODO = 'no TLSv1.3 sessions in LibreSSL'
> > - if $t->has_module('LibreSSL') && test_tls13();
> > + if ($t->has_module('LibreSSL')
> > + || Net::SSLeay::constant("LIBRESSL_VERSION_NUMBER"))
> > + && test_tls13();
> >
> > is(test_reuse(8443), 1, 'tickets reused');
> > is(test_reuse(8444), 1, 'tickets and cache reused');
> >
> > But I see little to no purpose: if the testing tool is broken
> > in various unexpected ways (another example is X509_V_ERR_INVALID_PURPOSE
> > in peer certificate verification as reported in the adjacent thread),
> > I think we barely can handle this in general.
>
> I generally agree.
>
> Still, the X509_V_ERR_INVALID_PURPOSE seems to be an OpenSSL
> 3.2.0-related issue: for tests using CA root certificates without
> CA:TRUE it now generates X509_V_ERR_INVALID_CA on the root
> certificate, which then changed to X509_V_ERR_INVALID_PURPOSE.
>
> Given the list of incompatible changes from NEWS.md, and the fact
> that the same tests work fine with OpenSSL 3.2.0 but with
> "openssl" binary from older versions, it seems to be this:
>
> * The `x509`, `ca`, and `req` apps now always produce X.509v3 certificates.
>
> This needs to be addressed.
Patch:
# HG changeset patch
# User Maxim Dounin
# Date 1706477656 -10800
# Mon Jan 29 00:34:16 2024 +0300
# Node ID 156665421f83a054cf331e8f9a27dd4d2f86114d
# Parent 27a7
Re: nginx-tests: Some SSL tests are failing with openssl 3.2.0
Hello! On Fri, Jan 26, 2024 at 04:28:15PM -0300, Renato Botelho wrote: > I'm building nginx on an environment with openssl 3.2.0 and some SSL tests > are failing. I suspect it's related to openssl version because it works on > another env with older openssl. But maybe I'm wrong. That's a result of incompatible change introduced in the "openssl" application by OpenSSL 3.2.0, it now generates X509v3 certs even if not asked to, just discussed here: https://mailman.nginx.org/pipermail/nginx-devel/2024-January/32O7PUI3XJZZGBMLS2NAH654MS23MVDD.html Will be eventually fixed. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: nginx-tests SSL tests failing out of the box?
Hello!
On Fri, Jan 26, 2024 at 09:29:58PM +0400, Sergey Kandaurov wrote:
> On Thu, Jan 25, 2024 at 11:38:57PM +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Thu, Jan 25, 2024 at 06:59:36PM +, Mayerhofer, Austin via
> > nginx-devel wrote:
> >
> > > Hi all,
> > >
> > > I have not made any changes to NGINX. Vanilla NGINX (./configure with no
> > > flags) passes all tests that run, but when compiling with SSL, not all
> > > SSL tests are passing. Is this expected, or do I need to configure nginx
> > > further aside from adding the --with-http_ssl_module flag? Do each of the
> > > failing tests below require separate fixes, or is there a
> > > one-size-fits-all solution for all of them?
> > >
> > > OS: MacOS 12.6.3
> > > Chip: Apple M1 Max
> > > NGINX: 1.24.0 built from source code with ./configure --with-debug
> > > --with-http_ssl_module
> > > Nginx-tests:
> > > https://github.com/nginx/nginx-tests/tree/4c2ad8093952706f327d04887c5546bad91b75a6
> > > OpenSSL: 3.2.0 (/opt/homebrew/bin/openssl)
> > > Perl: 5.30.3 (/usr/bin/perl)
> > >
> > > When I run
> > >
> > > ```
> > > TEST_NGINX_BINARY=/usr/local/nginx/sbin/nginx prove -v ssl.t
> > > ```
> > >
> > > I see
> > >
> > > ```
> > > not ok 2 - session reused
> > >
> > > # Failed test 'session reused'
> > > # at ssl.t line 187.
> > > # 'HTTP/1.1 200 OK
> > > # Server: nginx/1.24.0
> > > # Date: Thu, 25 Jan 2024 18:50:10 GMT
> > > # Content-Type: text/plain
> > > # Content-Length: 6
> > > # Connection: close
> > > #
> > > # body .'
> > > # doesn't match '(?^m:^body r$)'
> > > ```
> >
> > [...]
> >
> > It looks like SSL session reuse is broken in Perl you are
> > using. This might be the case if, for example, Net::SSLeay in
> > your installation was compiled with system LibreSSL as an SSL
> > library - at least on the server side LibreSSL simply does not
> > support session reuse with TLSv1.3.
> >
> > Test suite checks if nginx was compiled with LibreSSL and marks
> > appropriate tests as TODO, but if the Perl module is broken
> > instead, the test will fail.
> >
>
> Well, technically, we could test this and skip appropriately:
>
> diff --git a/ssl_session_reuse.t b/ssl_session_reuse.t
> --- a/ssl_session_reuse.t
> +++ b/ssl_session_reuse.t
> @@ -166,7 +166,9 @@ local $TODO = 'no TLSv1.3 sessions, old
> local $TODO = 'no TLSv1.3 sessions, old IO::Socket::SSL'
> if $IO::Socket::SSL::VERSION < 2.061 && test_tls13();
> local $TODO = 'no TLSv1.3 sessions in LibreSSL'
> - if $t->has_module('LibreSSL') && test_tls13();
> + if ($t->has_module('LibreSSL')
> + || Net::SSLeay::constant("LIBRESSL_VERSION_NUMBER"))
> + && test_tls13();
>
> is(test_reuse(8443), 1, 'tickets reused');
> is(test_reuse(8444), 1, 'tickets and cache reused');
>
> But I see little to no purpose: if the testing tool is broken
> in various unexpected ways (another example is X509_V_ERR_INVALID_PURPOSE
> in peer certificate verification as reported in the adjacent thread),
> I think we barely can handle this in general.
I generally agree.
Still, the X509_V_ERR_INVALID_PURPOSE seems to be an OpenSSL
3.2.0-related issue: for tests using CA root certificates without
CA:TRUE it now generates X509_V_ERR_INVALID_CA on the root
certificate, which then changed to X509_V_ERR_INVALID_PURPOSE.
Given the list of incompatible changes from NEWS.md, and the fact
that the same tests work fine with OpenSSL 3.2.0 but with
"openssl" binary from older versions, it seems to be this:
* The `x509`, `ca`, and `req` apps now always produce X.509v3 certificates.
This needs to be addressed.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Enforce that CR precede LF in chunk lines
Hello! On Thu, Jan 25, 2024 at 08:32:17PM +, Ben Kallus wrote: > > Still, there is a robustness principle which allows applications > > to parse requests with various deviations from the grammar. > > Whether this is principle is good is a matter of opinion. I tend to > lean toward thinking that it is not (as you can probably tell) but > reasonable minds will differ on this point. > > > You may want to be more specific what "off by one" means here. > > Happy to :) > > Here's an example of a payload that smuggles a request past Apache > Traffic Server to a Node.js backend: > ``` > POST / HTTP/1.1\r\n > Transfer-Encoding: chunked\r\n > \r\n > 2\r\r > ;a\r\n > 02\r\n > 2d\r\n > 0\r\n > \r\n > DELETE / HTTP/1.1\r\n > Content-Length: 183\r\n > \r\n > 0\r\n\r\nGET / HTTP/1.1\r\n\r\n > ``` > The exact mechanism of this payload is relatively unimportant (it has > to do with the `2\r\r;a`). The important point is that the POST is > seen by both ATS and Node, the DELETE is seen only by Node, and the > GET is seen only by ATS. Thus, the DELETE request is smuggled. > > (A very similar attack worked on Google Cloud's classic application > load balancer, and on Akamai's load balancer until very recently when > companies patched the bugs. I'm still working on the writeup for those > bugs, but you can see us present the material here: > https://yewtu.be/watch?v=aKPAX00ft5s=2h19m0s) > > You'll notice that the DELETE request has a Content-Length header. > This is because in order for the smuggling to go undetected, the > response to the DELETE request needs to be sent only after the GET > request is forwarded. One way to do this is to add a message body to > the DELETE request, so that it remains incomplete until the arrival of > the GET request. It is therefore necessary for the attacker to predict > the length of the GET request after it has passed through the reverse > proxy, so that this length can be used to compute the Content-Length > (or chunk size) in the DELETE request. Because reverse proxies often > modify requests, this is not always straightforward. > > In this instance, I use a Content-Length header of 183 because with > the configuration of ATS that I was attacking, `GET / > HTTP/1.1\r\n\r\n` ends up becoming 178 bytes long due to the insertion > of X-Forwarded-For, Via, etc., +5 for `0\r\n\r\n`. If I had used a > length less than 183, then Node would send a 400 after responding to > the DELETE request, which makes the reverse proxy aware that request > smuggling has occurred. If I had used a length greater than 183, then > Node would time out waiting for the rest of the DELETE request's > message body. Thus, I need to guess the length exactly right to pull > off undetected request smuggling. Guessing correctly can be > challenging, especially when added headers have unpredictable lengths. > This is common with CDNs, which often insert random identifiers into > request headers. > > If instead of using Content-Length, I had used a chunked message body > to smuggle the DELETE request, and the backend server allows bare LF > as a chunk line terminator, then my length guess could be one less > than the correct value without invalidating the message for servers > that accept bare LF in chunk lines. Thus, when developing future > request smuggling attacks, getting my length guess correct is a little > easier when the backend server allows bare LF chunk line endings. As far as I understand what goes on here and what do you mean by using a chunked message body, with length guess which is one less than the correct value you'll end up with LF + "\r\n0\r\n\r\n" at the request end, which will result in 400. Length which is one more will work though, so understood, thanks. In the original exploit length which is two less should work though, due to the "SHOULD ignore at least one empty line (CRLF) received prior to the request-line" robustness exception in 2.2. Message parsing of RFC 9112. And one less might also work, as long as empty lines before the request-line accept bare LF (not sure about Node though). Overall, I don't think there is a big difference here. > > including manual testing such > > as with nc(1). > > If you pass the -C flag, nc will translate LF to CRLF for you :) It won't, because "-C" is a non-portable flag provided by a Debian-specific patch. And even if it will work for some, this will still complicate testing. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: nginx-tests SSL tests failing out of the box?
Hello! On Thu, Jan 25, 2024 at 06:59:36PM +, Mayerhofer, Austin via nginx-devel wrote: > Hi all, > > I have not made any changes to NGINX. Vanilla NGINX (./configure with no > flags) passes all tests that run, but when compiling with SSL, not all SSL > tests are passing. Is this expected, or do I need to configure nginx further > aside from adding the --with-http_ssl_module flag? Do each of the failing > tests below require separate fixes, or is there a one-size-fits-all solution > for all of them? > > OS: MacOS 12.6.3 > Chip: Apple M1 Max > NGINX: 1.24.0 built from source code with ./configure --with-debug > --with-http_ssl_module > Nginx-tests: > https://github.com/nginx/nginx-tests/tree/4c2ad8093952706f327d04887c5546bad91b75a6 > OpenSSL: 3.2.0 (/opt/homebrew/bin/openssl) > Perl: 5.30.3 (/usr/bin/perl) > > When I run > > ``` > TEST_NGINX_BINARY=/usr/local/nginx/sbin/nginx prove -v ssl.t > ``` > > I see > > ``` > not ok 2 - session reused > > # Failed test 'session reused' > # at ssl.t line 187. > # 'HTTP/1.1 200 OK > # Server: nginx/1.24.0 > # Date: Thu, 25 Jan 2024 18:50:10 GMT > # Content-Type: text/plain > # Content-Length: 6 > # Connection: close > # > # body .' > # doesn't match '(?^m:^body r$)' > ``` [...] It looks like SSL session reuse is broken in Perl you are using. This might be the case if, for example, Net::SSLeay in your installation was compiled with system LibreSSL as an SSL library - at least on the server side LibreSSL simply does not support session reuse with TLSv1.3. Test suite checks if nginx was compiled with LibreSSL and marks appropriate tests as TODO, but if the Perl module is broken instead, the test will fail. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Enforce that CR precede LF in chunk lines
Hello! On Thu, Jan 25, 2024 at 03:21:16AM +, Ben Kallus wrote: > The HTTP/1.1 standards allow the recognition of bare LF as a line > terminator in some contexts. > > From RFC 9112 Section 2.2: > > Although the line terminator for the start-line and fields is the sequence > > CRLF, a recipient MAY recognize a single LF as a line terminator and ignore > > any preceding CR. > > From RFC 7230 Section 3.5: > > Although the line terminator for the start-line and header fields is > > the sequence CRLF, a recipient MAY recognize a single LF as a line > > terminator and ignore any preceding CR. > > From RFC 2616 Section 19.3: > > The line terminator for message-header fields is the sequence CRLF. > > However, we recommend that applications, when parsing such headers, > > recognize a single LF as a line terminator and ignore the leading CR. > > In summary, bare LF can be recognized as a line terminator for a > field-line (i.e. a header or trailer) or a start-line, but not outside > of these contexts. In particular, bare LF is not an acceptable line > terminator for chunk data lines or chunk size lines. One of the > rejection messages for an RFC 9112 errata report makes the reasoning > behind this choice clear: > > > The difference was intentional. A chunked parser is not a start line or > > field parser (it is a message body parser) and it is supposed to be less > > forgiving because it does not have to retain backwards compatibility with > > 1.0 parsers. > > Hence, bare LF around the chunk sizes would be invalid and should result in > > the connection being marked as invalid. Still, there is a robustness principle which allows applications to parse requests with various deviations from the grammar. Quoting RFC 2616: Although this document specifies the requirements for the generation of HTTP/1.1 messages, not all applications will be correct in their implementation. We therefore recommend that operational applications be tolerant of deviations whenever those deviations can be interpreted unambiguously. As such, it is certainly valid for a HTTP/1.1 server based on RFC 2616 to accept LF as line terminator in chunk sizes. While RFC 7230 and RFC 9112 tried to harden requirements, and now say that "the server SHOULD respond with 400" on deviations which are not explicitly allowed, it is still not a violation to accept such deviations. > Currently, Nginx allows chunk lines (both size and data) to use bare > LF as a line terminator. This means that (for example) the following > payload is erroneously accepted: > ``` > POST / HTTP/1.1\r\n > Host: whatever\r\n > Transfer-Encoding: chunked\r\n > 0\n# <--- This is > missing a \r > \r\n > ``` > > This is probably not such a big deal, but it is a standards violation, > and it comes with one small consequence: chunk lengths that are off by > one will not invalidate the message body. > > I've developed a few request smuggling exploits against other servers > and proxies in the past that rely upon the attacker's ability to > predict the length of a request after it has passed through a reverse > proxy. This is usually straightforward, but if there are any > unpredictable headers inserted by the proxy, getting the guess right > becomes nontrivial. Being able to be off by one thus makes the > attacker's job a little bit easier. You may want to be more specific what "off by one" means here. While it is true that an attacker which isn't able to generate proper CRLF for some reason might be stopped by such a restriction, it still needs to ensure there is an LF, which makes things mostly equivalent in almost all cases. > Given that many popular HTTP implementations (Apache httpd, Node, > Boost::Beast, Lighttpd) adhere to the standard on this line > termination issue, we should expect this change to break almost no > clients, since any client generating requests that terminate chunk > lines with bare LF would already be incompatible with a large portion > of the web. > > It is, however, also true that many HTTP implementations (Go net/http, > H2O, LiteSpeed) exhibit the same behavior as Nginx, and it's probably > worth exploring why that is. > > The following patch changes Nginx's parsing behavior to match the > standard. Note that this patch does not stop Nginx from allowing bare > LF in request lines, response lines, headers, or trailers. It stops > Nginx from accepting bare LF only in chunk size and data lines, where > the standard does not permit LF/CRLF permissiveness. It's also a > delete-only patch, which is always nice :) > > If you all are open to this change, it will also be neces
Re: [PATCH] SSL: Added SSLKEYLOGFILE key material to debug logging
Hello! On Sun, Jan 21, 2024 at 10:37:24AM +, J Carter wrote: > # HG changeset patch > # User J Carter > # Date 1705832811 0 > # Sun Jan 21 10:26:51 2024 + > # Node ID b00332a5253eefb53bacc024c72f55876c2eac6e > # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07 > SSL: Added SSLKEYLOGFILE key material to debug logging. > > This patch also introduces the debug_keylog error log level flag, which > may be used to graunually enable or ommit logging of key material via > error level flags (note, it's always enabled when using > debug_connection). > > Each line of key material is output to the error log as separate log > message, and is prepended with 'ssl keylog: ' for convenient extraction. > > The purpose of logging key material is to allow external tools, such as > wireshark/tshark, to decrypt captured TLS connections in all situations. > > Previously, only TLS 1.2 (and below) connections could be decrypted > when specific ciphers suites were used, and when the decrypter had > access to the acting server's TLS certificates and keys. It was not > possible to decrypt TLS 1.3 traffic without generating SSLKEYLOGFILE on > peer, or by using other hacks on nginx host (using GDB, or patched ssl > libraries). Thanks for the patch. Logging session keying material is known to be problematic from ethical point of view. As such, I would rather avoid introducing relevant functionality in nginx. [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
_stream_limit_conn_module.o | 105
stream/ngx_stream_log_module.o| 872 ++--
stream/ngx_stream_proxy_module.o | 749 ++--
stream/ngx_stream_script.o| 16
31 files changed, 10658 insertions(+), 10549 deletions(-)
In particular, in the only real change in ngx_palloc.o assembler
is as follows:
@@ -452,16 +452,19 @@ ngx_reset_pool:
testl %ebx, %ebx
jne .L99 .L97:
+ testl %esi, %esi
+ je .L100
movl%esi, %eax
.p2align 4,,10
.p2align 3
Which corresponds to restored with -fno-delete-null-pointer-check
initial "p" check on the first iteration of the second loop, due
to "pool->large" being used in the first loop initialization:
for (l = pool->large; l; l = l->next) {
if (l->alloc) {
ngx_free(l->alloc);
}
}
for (p = pool; p; p = p->d.next) {
p->d.last = (u_char *) p + sizeof(ngx_pool_t);
p->d.failed = 0;
}
Many other cases, such as multiple changes in ngx_inet.o, one of
the two changes in ngx_open_file_cache.o, and one change
ngx_http_parse.o, correspond to ngx_strlchr() calls, where
subsequent result check is omitted because no-match is directly
handled by the inlined ngx_strlchr() code:
p = ngx_strlchr(uri->data, last, '?');
if (p) { ... }
And there seems to be no real changes in ngx_http_script.o and
ngx_stream_script.o, just some minor instructions reordering.
Overall, it might be feasible to review all the differences to
ensure there are no real issues introduced by various compiler
optimizations.
[...]
> There is a proposal for C2y to define memcpy(NULL,NULL,0):
> https://docs.google.com/document/d/1guH_HgibKrX7t9JfKGfWX2UCPyZOTLsnRfR6UleD1F8/edit
> If you feel strongly that memcpy from NULL should be defined, feel
> free to contribute to it :)
Interesting, thanks. This would be the best solution and will
eliminate the need for any changes by defining the behaviour, as
it should.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Year 2024
Hello! On Tue, Jan 23, 2024 at 12:56:47PM +, Yaroslav Zhuravlev wrote: > GNUmakefile | 2 +- > text/LICENSE | 2 +- > xml/menu.xml | 1 + > 3 files changed, 3 insertions(+), 2 deletions(-) > > > # HG changeset patch > # User Yaroslav Zhuravlev > # Date 1706014530 0 > # Tue Jan 23 12:55:30 2024 + > # Node ID 9fa58eaa9b8fd4dcd9677d37a86140f945eaf7c6 > # Parent 87d313e1bf7f365ac81693aae5d83907869774cb > Year 2024. > > diff --git a/GNUmakefile b/GNUmakefile > --- a/GNUmakefile > +++ b/GNUmakefile > @@ -71,7 +71,7 @@ > > YEARS = \ > 2009 2010 2011 2012 2013 2014 2015 2016 2017 2018 2019 \ > - 2020 2021 2022 > + 2020 2021 2022 2023 > > all: news arx 404 $(LANGS) > > diff --git a/text/LICENSE b/text/LICENSE > --- a/text/LICENSE > +++ b/text/LICENSE > @@ -1,6 +1,6 @@ > /* > * Copyright (C) 2002-2021 Igor Sysoev > - * Copyright (C) 2011-2023 Nginx, Inc. > + * Copyright (C) 2011-2024 Nginx, Inc. > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > diff --git a/xml/menu.xml b/xml/menu.xml > --- a/xml/menu.xml > +++ b/xml/menu.xml > @@ -59,6 +59,7 @@ > --> > > news > + > > > Looks good. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Documented opensourcing of the OTel module
Hello! On Tue, Jan 23, 2024 at 01:49:07PM +, Yaroslav Zhuravlev wrote: > xml/en/docs/index.xml | 8 +++- > xml/en/docs/ngx_otel_module.xml | 20 +++- > xml/ru/docs/index.xml | 10 -- > 3 files changed, 30 insertions(+), 8 deletions(-) > > > # HG changeset patch > # User Yaroslav Zhuravlev > # Date 1704815768 0 > # Tue Jan 09 15:56:08 2024 + > # Node ID 00807e94be3622a79d7796be6ea11934f97b2662 > # Parent e3116677300fa455200da63002c746aece689029 > Documented opensourcing of the OTel module. > > diff --git a/xml/en/docs/index.xml b/xml/en/docs/index.xml > --- a/xml/en/docs/index.xml > +++ b/xml/en/docs/index.xml > @@ -8,7 +8,7 @@ > link="/en/docs/" > lang="en" > - rev="49" > + rev="50" > toc="no"> > > > @@ -681,6 +681,12 @@ > ngx_mgmt_module > > > + > + > + > + > + > + > > > ngx_otel_module > diff --git a/xml/en/docs/ngx_otel_module.xml b/xml/en/docs/ngx_otel_module.xml > --- a/xml/en/docs/ngx_otel_module.xml > +++ b/xml/en/docs/ngx_otel_module.xml > @@ -9,12 +9,14 @@ >link="/en/docs/ngx_otel_module.html" > lang="en" > -rev="1"> > +rev="2"> > > > > > -The ngx_otel_module module (1.23.4) provides > +The ngx_otel_module module (1.23.4) is nginx-authored Quoting from https://mailman.nginx.org/pipermail/nginx-devel/2023-October/4AGH5XVKNP6UDFE32PZIXYO7JQ4RE37P.html: : Note that "nginx-authored" here looks misleading, as no nginx core : developers work on this module. > +https://github.com/nginxinc/nginx-otel;>third-party module > +that provides > https://opentelemetry.io;>OpenTelemetry > distributed tracing support. > The module supports > @@ -23,12 +25,20 @@ > > > > +The module is open source since 1.25.2. > +Download and install instructions are available > + url="https://github.com/nginxinc/nginx-otel/blob/main/README.md;>here. > +The module is also available as a prebuilt > +nginx-module-otel dynamic module > +package (1.25.4). > + > + > + > > This module is available as part of our > commercial subscription > -in nginx-plus-module-otel package. > -After installation, the module can be loaded > -dynamically. > +(the > + url="https://docs.nginx.com/nginx/admin-guide/dynamic-modules/opentelemetry;>nginx-plus-module-otel > package). I don't see reasons to provide additional links here. Rather, the note probably can be removed altogether, or changed to something like "In previuos versions, this module is available...". > > > > diff --git a/xml/ru/docs/index.xml b/xml/ru/docs/index.xml > --- a/xml/ru/docs/index.xml > +++ b/xml/ru/docs/index.xml > @@ -8,7 +8,7 @@ > link="/ru/docs/" > lang="ru" > - rev="49" > + rev="50" > toc="no"> > > > @@ -687,9 +687,15 @@ > ngx_mgmt_module [en] > > > + > + > + > + > + > + > > > -ngx_otel_module [en] > +ngx_otel_module [en] > > > > ___ > nginx-devel mailing list > nginx-devel@nginx.org > https://mailman.nginx.org/mailman/listinfo/nginx-devel -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Avoiding mixed socket families in PROXY protocol v1 (ticket #2594)
Hello! On Mon, Jan 22, 2024 at 07:48:01PM +0400, Roman Arutyunyan wrote: > Hi, > > On Mon, Jan 22, 2024 at 02:59:21PM +0300, Maxim Dounin wrote: > > Hello! > > > > On Mon, Jan 22, 2024 at 02:49:54PM +0400, Roman Arutyunyan wrote: > > > > > # HG changeset patch > > > # User Roman Arutyunyan > > > # Date 1705916128 -14400 > > > # Mon Jan 22 13:35:28 2024 +0400 > > > # Node ID 2f12c929527b2337c15ef99d3a4dc97819b61fbd > > > # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07 > > > Avoiding mixed socket families in PROXY protocol v1 (ticket #2594). Also nitpicking: ticket #2010 might be a better choice. The #2594 is actually a duplicate (with a side issue noted that using long unix socket path might result in a PROXY protocol header without ports and CRLF) and should be closed as such. > > > > > > When using realip module, remote and local addreses of a connection can > > > belong > > > to different address families. This previously resulted in generating > > > PROXY > > > protocol headers like this: > > > > > > PROXY TCP4 127.0.0.1 unix:/tmp/nginx1.sock 55544 0 > > > > > > The PROXY protocol v1 specification does not allow mixed families. The > > > change > > > will generate the unknown PROXY protocol header in this case: > > > > > > PROXY UNKNOWN > > > > > > Also, the above mentioned format for unix socket address is not specified > > > in > > > PROXY protocol v1 and is a by-product of internal nginx representation of > > > it. > > > The change eliminates such addresses from PROXY protocol headers as well. > > > > Nitpicking: double space in "from PROXY". > > Yes, thanks. > > > This change will essentially disable use of PROXY protocol in such > > configurations. While it is probably good enough from formal > > point of view, and better that what we have now, this might still > > be a surprise, especially when multiple address families are used > > on the original proxy server, and the configuration works for some > > of them, but not for others. > > > > Wouldn't it be better to remember if the PROXY protocol was used > > to set the address, and use $proxy_protocol_server_addr / > > $proxy_protocol_server_port in this case? > > > > Alternatively, we can use some dummy server address instead, so > > the client address will be still sent. > > Another alternative is duplicating client address in this case, see patch. I don't think it is a good idea. Using some meaningful real address might easily mislead users. I would rather use a clearly dummy address instead, such as INADDR_ANY with port 0. Also, as suggested, using the server address as obtained via PROXY protocol from the client might be a better solution as long as the client address was set via PROXY protocol (regardless of whether address families match or not), and what users expect from the "proty_protocol on;" when chaining stream proxies in the first place. [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Avoiding mixed socket families in PROXY protocol v1 (ticket #2594)
Hello! On Mon, Jan 22, 2024 at 02:49:54PM +0400, Roman Arutyunyan wrote: > # HG changeset patch > # User Roman Arutyunyan > # Date 1705916128 -14400 > # Mon Jan 22 13:35:28 2024 +0400 > # Node ID 2f12c929527b2337c15ef99d3a4dc97819b61fbd > # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07 > Avoiding mixed socket families in PROXY protocol v1 (ticket #2594). > > When using realip module, remote and local addreses of a connection can belong > to different address families. This previously resulted in generating PROXY > protocol headers like this: > > PROXY TCP4 127.0.0.1 unix:/tmp/nginx1.sock 55544 0 > > The PROXY protocol v1 specification does not allow mixed families. The change > will generate the unknown PROXY protocol header in this case: > > PROXY UNKNOWN > > Also, the above mentioned format for unix socket address is not specified in > PROXY protocol v1 and is a by-product of internal nginx representation of it. > The change eliminates such addresses from PROXY protocol headers as well. Nitpicking: double space in "from PROXY". This change will essentially disable use of PROXY protocol in such configurations. While it is probably good enough from formal point of view, and better that what we have now, this might still be a surprise, especially when multiple address families are used on the original proxy server, and the configuration works for some of them, but not for others. Wouldn't it be better to remember if the PROXY protocol was used to set the address, and use $proxy_protocol_server_addr / $proxy_protocol_server_port in this case? Alternatively, we can use some dummy server address instead, so the client address will be still sent. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 3] Stream: socket peek in preread phase
Hello!
On Fri, Jan 19, 2024 at 03:42:37PM +0400, Sergey Kandaurov wrote:
>
> > On 18 Jan 2024, at 23:44, Maxim Dounin wrote:
> >
> > Hello!
> >
> > On Thu, Jan 18, 2024 at 08:15:33PM +0400, Sergey Kandaurov wrote:
> >
> >> On Thu, Jan 18, 2024 at 07:06:06PM +0400, Roman Arutyunyan wrote:
> >>> Hi,
> >>>
> >>> # HG changeset patch
> >>> # User Roman Arutyunyan
> >>> # Date 1702476295 -14400
> >>> # Wed Dec 13 18:04:55 2023 +0400
> >>> # Node ID 7324e8e73595c3093fcc2cbd2b5d6b1a947be3b0
> >>> # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07
> >>> Stream: socket peek in preread phase.
> >>>
> >>> Previously, preread buffer was always read out from socket, which made it
> >>> impossible to terminate SSL on the connection without introducing
> >>> additional
> >>> SSL BIOs. The following patches will rely on this.
> >>>
> >>> Now, when possible, recv(MSG_PEEK) is used instead, which keeps data in
> >>> socket.
> >>> It's called if SSL is not already terminated and if an egde-triggered
> >>> event
> >>> method is used. For epoll, EPOLLRDHUP support is also required.
> >>>
> >>> diff --git a/src/stream/ngx_stream_core_module.c
> >>> b/src/stream/ngx_stream_core_module.c
> >>> --- a/src/stream/ngx_stream_core_module.c
> >>> +++ b/src/stream/ngx_stream_core_module.c
> >>> @@ -10,6 +10,11 @@
> >>> #include
> >>>
> >>>
> >>> +static ngx_uint_t ngx_stream_preread_can_peek(ngx_connection_t *c);
> >>> +static ngx_int_t ngx_stream_preread_peek(ngx_stream_session_t *s,
> >>> +ngx_stream_phase_handler_t *ph);
> >>> +static ngx_int_t ngx_stream_preread(ngx_stream_session_t *s,
> >>> +ngx_stream_phase_handler_t *ph);
> >>> static ngx_int_t ngx_stream_core_preconfiguration(ngx_conf_t *cf);
> >>> static void *ngx_stream_core_create_main_conf(ngx_conf_t *cf);
> >>> static char *ngx_stream_core_init_main_conf(ngx_conf_t *cf, void *conf);
> >>> @@ -203,8 +208,6 @@ ngx_int_t
> >>> ngx_stream_core_preread_phase(ngx_stream_session_t *s,
> >>> ngx_stream_phase_handler_t *ph)
> >>> {
> >>> -size_t size;
> >>> -ssize_t n;
> >>> ngx_int_trc;
> >>> ngx_connection_t*c;
> >>> ngx_stream_core_srv_conf_t *cscf;
> >>> @@ -217,56 +220,33 @@ ngx_stream_core_preread_phase(ngx_stream
> >>>
> >>> if (c->read->timedout) {
> >>> rc = NGX_STREAM_OK;
> >>> +goto done;
> >>> +}
> >>>
> >>> -} else if (c->read->timer_set) {
> >>> -rc = NGX_AGAIN;
> >>> +if (!c->read->timer_set) {
> >>> +rc = ph->handler(s);
> >>>
> >>> -} else {
> >>> -rc = ph->handler(s);
> >>> +if (rc != NGX_AGAIN) {
> >>> +goto done;
> >>> +}
> >>> }
> >>>
> >>> -while (rc == NGX_AGAIN) {
> >>> -
> >>> +if (c->buffer == NULL) {
> >>> +c->buffer = ngx_create_temp_buf(c->pool,
> >>> cscf->preread_buffer_size);
> >>> if (c->buffer == NULL) {
> >>> -c->buffer = ngx_create_temp_buf(c->pool,
> >>> cscf->preread_buffer_size);
> >>> -if (c->buffer == NULL) {
> >>> -rc = NGX_ERROR;
> >>> -break;
> >>> -}
> >>> +rc = NGX_ERROR;
> >>> +goto done;
> >>> }
> >>> -
> >>> -size = c->buffer->end - c->buffer->last;
> >>> -
> >>> -if (size == 0) {
> >>> -ngx_log_error(NGX_LOG_ERR, c->log, 0, "preread buffer full");
> >>> -rc = NGX_STREAM_BAD_REQUEST;
> >>> -break;
> >>> -}
> >>> +}
> >>>
> >>> -if (c->read->eof) {
> >>> -rc = NGX_STREAM_OK;
> >>> -break;
> >>> -}
> >>> -
> >
Re: [PATCH 1 of 3] Stream: socket peek in preread phase
eam
> > }
> >
> >
> > +static ngx_uint_t
> > +ngx_stream_preread_can_peek(ngx_connection_t *c)
> > +{
> > +#if (NGX_STREAM_SSL)
> > +if (c->ssl) {
> > +return 0;
> > +}
> > +#endif
> > +
> > +if ((ngx_event_flags & NGX_USE_CLEAR_EVENT) == 0) {
> > +return 0;
> > +}
> > +
> > +#if (NGX_HAVE_KQUEUE)
> > +if (ngx_event_flags & NGX_USE_KQUEUE_EVENT) {
> > +return 1;
> > +}
> > +#endif
> > +
> > +#if (NGX_HAVE_EPOLLRDHUP)
> > +if ((ngx_event_flags & NGX_USE_EPOLL_EVENT) && ngx_use_epoll_rdhup) {
> > +return 1;
> > +}
> > +#endif
> > +
> > +return 0;
> > +}
> > +
> > +
> > +static ngx_int_t
> > +ngx_stream_preread_peek(ngx_stream_session_t *s,
> > ngx_stream_phase_handler_t *ph)
> > +{
> > +ssize_tn;
> > +ngx_int_t rc;
> > +ngx_err_t err;
> > +ngx_connection_t *c;
> > +
> > +c = s->connection;
> > +
> > +n = recv(c->fd, (char *) c->buffer->last,
> > + c->buffer->end - c->buffer->last, MSG_PEEK);
> > +
> > +err = ngx_socket_errno;
> > +
> > +ngx_log_debug1(NGX_LOG_DEBUG_STREAM, c->log, 0, "stream recv(): %z",
> > n);
> > +
> > +if (n == -1) {
> > +if (err == NGX_EAGAIN) {
> > +c->read->ready = 0;
> > +return NGX_AGAIN;
> > +}
> > +
> > +ngx_connection_error(c, err, "recv() failed");
> > +return NGX_STREAM_OK;
> > +}
> > +
> > +if (n == 0) {
> > +return NGX_STREAM_OK;
> > +}
> > +
> > +c->buffer->last += n;
> > +
> > +rc = ph->handler(s);
> > +
> > +if (rc != NGX_AGAIN) {
> > +c->buffer->last = c->buffer->pos;
> > +return rc;
> > +}
> > +
> > +if (c->buffer->last == c->buffer->end) {
> > +ngx_log_error(NGX_LOG_ERR, c->log, 0, "preread buffer full");
> > +return NGX_STREAM_BAD_REQUEST;
> > +}
> > +
> > +if (c->read->pending_eof) {
> > +return NGX_STREAM_OK;
> > +}
> > +
> > +c->buffer->last = c->buffer->pos;
> > +
> > +return NGX_AGAIN;
> > +}
> > +
> > +
> > +static ngx_int_t
> > +ngx_stream_preread(ngx_stream_session_t *s, ngx_stream_phase_handler_t *ph)
> > +{
> > +ssize_tn;
> > +ngx_int_t rc;
> > +ngx_connection_t *c;
> > +
> > +c = s->connection;
> > +
> > +while (c->read->ready) {
> > +
> > +n = c->recv(c, c->buffer->last, c->buffer->end - c->buffer->last);
> > +
> > +if (n == NGX_AGAIN) {
> > +return NGX_AGAIN;
> > +}
> > +
> > +if (n == NGX_ERROR || n == 0) {
> > +return NGX_STREAM_OK;
> > +}
> > +
> > +c->buffer->last += n;
> > +
> > +rc = ph->handler(s);
> > +
> > +if (rc != NGX_AGAIN) {
> > +return rc;
> > +}
> > +
> > +if (c->buffer->last == c->buffer->end) {
> > +ngx_log_error(NGX_LOG_ERR, c->log, 0, "preread buffer full");
> > +return NGX_STREAM_BAD_REQUEST;
> > +}
> > +}
> > +
> > +return NGX_AGAIN;
> > +}
> > +
> > +
> > ngx_int_t
> > ngx_stream_core_content_phase(ngx_stream_session_t *s,
> > ngx_stream_phase_handler_t *ph)
>
> Looks good.
I'm somewhat sceptical about the idea of functionality which
depends on the edge-triggered event methods being available.
If/when the patch series is reviewed, please make sure it gets my
approval before commit.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: This is a question about the "$status" log value when "proxy_read_timeout" occurs.
Hello! On Tue, Jan 16, 2024 at 01:15:09PM +0900, 박규철 wrote: > This is a question about the "$status" log value when "proxy_read_timeout" > occurs. > Nginx version in use: v1.25.3 > > Contents of 1Mbyte size were requested to [Origin Server]. > A response up to approximately 500Kbytes in size, including the header, was > received without delay. > However, after 500Kbytes, no response was received from Origin for 3 > seconds and the connection (time-out) > Since the message "upstream timed out...while reading upstream" was logged > in the error log, I think the connection was lost due to the > "proxy_read_timeout 3s" setting. > > While checking the log, I noticed that the "$status" value in the access > log was different from what I thought. > In my opinion, if the connection was terminated by "proxy_read_timeout", > the "$status" value would be 5xx, but the "$status" value in the saved > access log was 200. > > A normal response was not completed due to "proxy_read_timeout", so I would > like to know why the "$status" value is stored as 200 instead of 5xx. > Should I check a variable other than "$status" for responses to abnormal > timeouts such as "proxy_read_timeout"? The $status variable shows the status as sent to the client in the response headers. When proxy_read_timeout happens, the response headers are already sent, so $status contains 200 as sent to the client. For errors happened during sending the response body, consider looking into the error log. Some generic information about successful request completion might be found in the $request_completion variable (http://nginx.org/r/$request_completion). Note though that it might not be set for variety of reasons. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: processing a request without body
Hello! On Sat, Jan 13, 2024 at 03:11:11PM +0800, Muhammad Nuzaihan wrote: > Hi Maxim, > > I did enable debug logs before i posted the question. > > With json payload, my code is executed right after doing malloc and > "http request body content length filter". > > Without a json payload, it doesn't execute my request > validation.That's why i thought it might be due to content length is > 0. > > Here is the debug log when i curl with an empty payload: > > 2024/01/13 15:01:19 [debug] 2452969#0: *11 rewrite phase: 0 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 test location: "/" > 2024/01/13 15:01:19 [debug] 2452969#0: *11 test location: "proxy/health" > 2024/01/13 15:01:19 [debug] 2452969#0: *11 test location: "proxy/unhealthy" > 2024/01/13 15:01:19 [debug] 2452969#0: *11 test location: > "proxy/profile/alice/comment" > 2024/01/13 15:01:19 [debug] 2452969#0: *11 using configuration > "/proxy/profile/alice/comment" > 2024/01/13 15:01:19 [debug] 2452969#0: *11 http cl:0 max:1048576 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 rewrite phase: 2 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 post rewrite phase: 3 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 generic phase: 4 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 http request body content > length filter > 2024/01/13 15:01:19 [debug] 2452969#0: *11 http read client request body Clearly request body reading is called here, at the preaccess phase. This implies that your code is called - nginx itself won't try to read the request body that early. Everything else is up to your code. > 2024/01/13 15:01:19 [debug] 2452969#0: *11 recv: eof:0, avail:0 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 http client request body recv -2 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 http client request body rest 1 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 event timer add: 3: 6:183385517 Note that request body reading code blocks waiting for more data. > 2024/01/13 15:01:19 [debug] 2452969#0: *11 http finalize request: -4, > "/proxy/profile/alice/comment?" a:1, c:2 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 http request count:2 blk:0 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 access phase: 7 > 2024/01/13 15:01:19 [debug] 2452969#0: *11 access phase: 8 Note that phase handling continues here: it shouldn't, since request body reading is in progress. This suggested that your code fails to stop phase handling after calling ngx_http_read_client_request_body(): note you should return NGX_DONE to stop processing unless there is an immediate error. [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] fastcgi_params: added REMOTE_HOST parameter
Hello!
On Fri, Jan 12, 2024 at 11:03:45PM +, Jakub Zelenka wrote:
> On Fri, Jan 12, 2024 at 10:20 PM Maxim Dounin wrote:
>
> > > # HG changeset patch
> > > # User Jakub Zelenka
> > > # Date 1705078404 0
> > > # Fri Jan 12 16:53:24 2024 +
> > > # Node ID 1ff2f737bd318a730d0944a6037c8fd7c7da2656
> > > # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07
> > > Added REMOTE_HOST parameter to fastcgi_params.
> > >
> > > When HTTP/3 is used, users will no longer get HTTP_HOST as host header
> > is no
> > > longer set by most clients. It is useful / necessary for many setups to
> > have
> > > such information and REMOTE_HOST is defined in CGI/1.1 for such purpose.
> >
> > https://datatracker.ietf.org/doc/html/rfc3875#section-4.1.9
> >
> >The REMOTE_HOST variable contains the fully qualified domain name of
> >the client sending the request to the server, if available, otherwise
> >NULL.
> >
> > That is, REMOTE_HOST is completely unrelated. It is not the
> > hostname of the requested server, but the hostname of the client -
> > result of a reverse DNS lookup of a client's IP address, something
> > used to be provided by some servers when Internet was small (e.g,
> > HostnameLookups in Apache). It is certainly not the right param
> > to use for $host.
> >
> >
> I think you are right. I somehow thought about nginx as a client for some
> reason (technically it's a client from FPM point of view) but I agree that
> the meaning is different here.
>
> > IMO, proper param to use would be SERVER_NAME. It is set to
> > $server_name by default, though can be modified locally to provide
> > $host if needed in the particular configuration.
>
> I think it's probably the best option. Although current default value is a
> bit unfortunate as it's just server_name specified by user so most of the
> time is meaningless. Not sure if it can really comply with linked RFC
> either as it doesn't have to be hostname. On the other side it's been
> default for ages so I guess it won't be changed, right?
Well, $server_name is actually expected to be a proper/canonical
name of the server. In particular, it is used by nginx itself
when returning redirects with "server_name_in_redirect on;" (used
to be the default, but not anymore, since the default server_name
was changed to "" in nginx 0.8.48).
I believe it is up to the particular server configuration if
SERVER_NAME should be set to $server_name, that is, the canonical
name of the server, or should use $host, as might be preferred in
more dynamic configurations where multiple names are handled in a
single server{} block or when server_name is not set at all.
Changing the default as provided in various fastcgi_param files as
shipped with nginx might be indeed troublesome though. For
example, if a configuration expects SERVER_NAME to be a canonical
name, changing SERVER_NAME to client-controlled $host might result
in security issues.
> It's all just a bit unfortunate because with HTTP/3, there is no longer any
> server host info in the default configuration that would be passed to FPM.
The "Host" header is not required to be present in HTTP/1.x
either, and even in HTTP/1.1 where it is required to be present,
it might not match the authority actually requested via the
request line. That is, technically using the "Host" header for
anything, except might be links sent to the client itself, is
almost always wrong.
OTOH, for HTTP/2 nginx emulates the "Host" header based on the
":authority" pseudo-header. Given the amount of pain the approach
taken for HTTP/3 causes, we might consider doing the same for
HTTP/3 as well.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: processing a request without body
Hello! On Fri, Jan 12, 2024 at 10:17:42PM +0800, Muhammad Nuzaihan wrote: > Hi Maxim, > > Thank you so much for your explaination. > > I have another question. If i have an empty string in my payload, it > skips the phase handler completely. > > Example: curl -X POST http://localhost/proxy/profile/alice/comment -d > '' -H 'Content-Type: application/json' > > the flag "-d ''" > > I do doing it at NGX_HTTP_ACCESS_PHASE in the handler. It seems that > if "content_length = 0", it skips the access phase handler as well? Access phase handlers are called for all requests (unless these are rejected at earlier stages). If in doubt, consider configuring debug logging and add appropriate debug logging to your module, it should make things obvious enough. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] fastcgi_params: added REMOTE_HOST parameter
Hello! On Fri, Jan 12, 2024 at 05:04:22PM +, Jakub Zelenka wrote: > Hi, > > I'm a PHP-FPM maintainer and some FPM users have issues with missing host > header when using HTTP/3: https://github.com/php/php-src/issues/13021 . > This is not an nginx issue as correctly noted in > https://trac.nginx.org/nginx/ticket/2281 but it would be nice to have > fastcgi_param set for getting host in default config. I was thinking how to > best expose $host and REMOTE_HOST seems logical and so I think it could be > useful addition. I can update FPM to also set REMOTE_HOST from HTTP_HOST if > REMOTE_HOST is not set which would make it even more available for HTTP/1.1 > and HTTP/2 users. > > Please let me know what you think! > > # HG changeset patch > # User Jakub Zelenka > # Date 1705078404 0 > # Fri Jan 12 16:53:24 2024 + > # Node ID 1ff2f737bd318a730d0944a6037c8fd7c7da2656 > # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07 > Added REMOTE_HOST parameter to fastcgi_params. > > When HTTP/3 is used, users will no longer get HTTP_HOST as host header is no > longer set by most clients. It is useful / necessary for many setups to have > such information and REMOTE_HOST is defined in CGI/1.1 for such purpose. https://datatracker.ietf.org/doc/html/rfc3875#section-4.1.9 The REMOTE_HOST variable contains the fully qualified domain name of the client sending the request to the server, if available, otherwise NULL. That is, REMOTE_HOST is completely unrelated. It is not the hostname of the requested server, but the hostname of the client - result of a reverse DNS lookup of a client's IP address, something used to be provided by some servers when Internet was small (e.g, HostnameLookups in Apache). It is certainly not the right param to use for $host. IMO, proper param to use would be SERVER_NAME. It is set to $server_name by default, though can be modified locally to provide $host if needed in the particular configuration. > > diff -r ee40e2b1d083 -r 1ff2f737bd31 conf/fastcgi_params > --- a/conf/fastcgi_params Mon Dec 25 21:15:48 2023 +0400 > +++ b/conf/fastcgi_params Fri Jan 12 16:53:24 2024 + > @@ -17,6 +17,7 @@ > > fastcgi_param REMOTE_ADDR$remote_addr; > fastcgi_param REMOTE_PORT$remote_port; > +fastcgi_param REMOTE_HOST$host; > fastcgi_param SERVER_ADDR$server_addr; > fastcgi_param SERVER_PORT$server_port; > fastcgi_param SERVER_NAME$server_name; -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: processing a request without body
Hello!
On Fri, Jan 12, 2024 at 03:26:39AM +0800, Muhammad Nuzaihan wrote:
> Hi Maxim,
>
> After searching the archives, I found the solution which you had
> answered before:
> https://www.ruby-forum.com/t/trouble-getting-the-request-body-of-a-http-post/180463/4
>
> The code that reads the body is:
> rc = ngx_http_read_client_request_body(r,
> ngx_http_foo_body_handler);
>
> if (rc >= NGX_HTTP_SPECIAL_RESPONSE) {
> return rc;
> }
>
> Can i buy you coffee?
You may want to start with basic examples and the dev guide,
specifically:
https://nginx.org/en/docs/dev/development_guide.html#http_request_body
Note though that reading the request body during phase processing,
in contrast to doing so in a content handler, requires special
precautions. In particular, you'll have to call
ngx_http_finalize_request(NGX_DONE) to ensure correct request
reference counting:
https://hg.nginx.org/nginx/file/tip/src/http/modules/ngx_http_mirror_module.c#l120
Further, to resuming phase processing after reading the request
body you'll have to restore r->write_event_handler and call
ngx_http_core_run_phases(). See here in the mirror module for an
example:
https://hg.nginx.org/nginx/file/tip/src/http/modules/ngx_http_mirror_module.c#l144
Hope this helps.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello! On Tue, Jan 09, 2024 at 04:18:06PM +, Ben Kallus wrote: > > This demonstrates that your patch > > is clearly insufficient. Further, Vladimir's patch is clearly > > insufficient too, as shown for the another patch in the same > > patch series. > > "Insufficient" only when compared to a hypothetical perfectly exhaustive > patch that requires "huge work," as you put it. It's best not to let the > perfect be the enemy of the good. > > Avoiding UB in normal program execution (as opposed to the test suite) will > prevent common workloads from executing UB, which is not merely an issue of > "theoretical correctness." See https://blog.regehr.org/archives/213 > (section "A Fun Case Analysis") for an example of how this "NULL used in > nonnull context" issue leads to unexpected program behavior. > > Thus, I think the best approach is to patch pstrdup to avoid > memcpy-from-NULL, and patch other functions only if someone can present a > backtrace from a real configuration of nginx that executed UB. Thank you for your opinion. As I tried to explain in the review of Vladimir's patches, fixing scattered sanitizer reports individually, assuming no direct impact is present, has an obvious downside: as long as there is a consistent coding pattern which causes such reports, fixing individual reports will hide the pattern from being seen by the sanitizer, but won't eliminate it. As such, it will greatly reduce pressure on fixing the pattern, but if the pattern is indeed practically dangerous and has security consequences, it will be trivial for an attacker to find out cases which are not fixed and exploit them. As such, I prefer to identify patterns and fix them consistently over the code base instead of trying to quench individual reports. Quenching individual reports makes sense if we don't want to fix the pattern, assuming it is completely harmless anyway, but rather want to simplify usage of the sanitizer to identify other issues. This does not look like what you are advocating about though. (Also, again, patching just ngx_pstrdup() is clearly not enough even for this, see Vladimir's patch for a list of other places reported by UBSan in perfectly real configurations.) As already pointed out previously, there are no known cases when memcpy(p, NULL, 0) can result in miscompilation of nginx code, as nginx usually does not checks string data pointers against NULL (and instead checks length, if needed). In particular, ngx_pstrdup() you are trying to patch doesn't. That is, this is exactly the "no direct impact" situation as assumed above. If you think there are cases when the code can be miscompiled in practice, and not theoretically, please share. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 4 of 4] AIO operations now add timers (ticket #2162)
Hello! On Mon, Jan 08, 2024 at 01:31:11PM +, J Carter wrote: > On Mon, 8 Jan 2024 11:25:55 + > J Carter wrote: > > > Hello, > > > > On Mon, 27 Nov 2023 05:50:27 +0300 > > Maxim Dounin wrote: > > > > > # HG changeset patch > > > # User Maxim Dounin > > > # Date 1701050170 -10800 > > > # Mon Nov 27 04:56:10 2023 +0300 > > > # Node ID 00c3e7333145ddb5ea0eeaaa66b3d9c26973c9c2 > > > # Parent 61d08e4cf97cc073200ec32fc6ada9a2d48ffe51 > > > AIO operations now add timers (ticket #2162). > > > > > > Each AIO (thread IO) operation being run is now accompanied with 1-minute > > > timer. This timer prevents unexpected shutdown of the worker process > > > while > > > an AIO operation is running, and logs an alert if the operation is running > > > for too long. > > > > Shouldn't this timer's duration be set to match worker_shutdown_timeout's > > duration rather than being hard coded to 60s ? > > Ah nevermind, I understand. > > These timers will either expire from passing the 60s set duration, or > will expire as worker_process_timeout itself expires, kills the > connection and times out associated timers (including the aio timers). > > Setting it to worker_shutdown_timeout's duration would be pointless > (an 'infinite' timer would give the same result). > > So the only situation in which a different value for these AIO > timers would make sense is if these AIO operations are expected to > take longer 60s, but less than worker_shutdown_timeout (in cases > where it has been increased from it's own default of 60s). > > In that case the AIO operation's timeout would have to be one > (or more) of it's own directives, with a value less than > worker_shutdown_timeout. Not really. When worker_shutdown_timeout expires, it tries to terminate the request, but it can't as long as an AIO operation is running. When the AIO operation completes, the request will be actually terminated and the worker process will be allowed to exit. So far so good. But if the AIO operation never completes, the timer will expire after 1 minute, will log an alert, and the worker processes will be anyway allowed to exit (with the request still present). This might not be actually possible though - for example, ngx_thread_pool_exit_worker() will just block waiting for all pending operations to complete. In theory, the situation when an AIO operation never completes should never happen, and just a counter of pending AIO operations can be used instead to delay shutdown (which is essentially equivalent to an infinite timer). In practice, though, using a large enough guard timer might be better: it provides additional level of protection against bugs or system misbehaviour, and at least logs an alert if something really weird happens. It is also looks more universal and in line with current approach of using existing timers as an indicator that something is going on and shutdown should be delayed. The timer is expected to be "large enough", since we cannot do anything meaningful with an AIO operation which never completes, we can only complain loudly, so the timer should never expire unless there is something really wrong. This is not the case with worker_shutdown_timeout: it can be set to an arbitrary low value, which is not expected to mean that something is really wrong if the timer expires, but rather means that nginx shouldn't try to process remaining requests, but should instead close them as long as it can do so. That is, worker_shutdown_timeout does not fit semantically. Further, worker_shutdown_timeout is not set by default, so it simply cannot be used. The 1 minute was chosen as it matches default send_timeout, which typically accompanies AIO operations when sending responses (and also delays shutdown, so no "open socket left" alerts are normally seen). Still, send_timeout is also quite different semantically, and therefore a hardcoded value is used instead. I don't think there are valid cases when AIO operations can take longer than 1 minute, as these are considered to be small and fast operations, which are normally done synchronously within nginx event loop when not using AIO, and an operations which takes 1m would mean nginx is completely unresponsive. Still, if we'll found out that 1 minute is not large enough in some cases, we can just bump it to a larger value. [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] SSL: raised limit for upstream session size
Hello! On Tue, Dec 26, 2023 at 12:29:54AM +0400, Sergey Kandaurov wrote: > > On 23 Dec 2023, at 01:46, Maxim Dounin wrote: > > > > Hello! > > > > On Fri, Dec 22, 2023 at 06:28:34PM +0400, Sergey Kandaurov wrote: > > > >> # HG changeset patch > >> # User Sergey Kandaurov > >> # Date 1703255284 -14400 > >> # Fri Dec 22 18:28:04 2023 +0400 > >> # Node ID a463fb67e143c051fd373d1df94e5813a37d5cea > >> # Parent 44266e0651c44f530c4aa66e68c1b9464a9acee7 > >> SSL: raised limit for upstream session size. > >> > >> Unlike shared session cache used to store multiple client SSL sessions and > >> which may be per a single SSL connection, sessions saved from upstream are > >> per upstream server peer, so there is no such multiplier effect, but they > >> may be of noticeably larger size due to session tickets being used. > >> > >> It was observed that session tickets sent from JVM backends may result in > >> a decoded session size nearly the previous maximum session size limit of > >> 4096 or slightly beyond. Raising the limit allows to save such sessions. > > > > Session tickets are not expected to be larger than sessions > > itself, except by several bytes used for key identification and > > encryption overhead. I see no reasons why the limit should be > > different in different places. > > > > And 4096 for an SSL session looks a lot. The only justification I > > can assume here is an SSL session with the client certificate (or > > even certificate chain) being saved into the session. It might > > worth looking into what actually happens here. > > > > Indeed. Both local and peer certificate chains are serialized and > encrypted as part of constructing a session ticket. Per the original > change to support tickets, this is hardcoded and may not be adjusted: > https://hg.openjdk.org/jdk/jdk/rev/c2398053ee90#l4.352 > https://hg.openjdk.org/jdk/jdk/rev/c2398053ee90#l10.261 From my limited understanding of the JDK code, at least peerCerts seems to contain only certificates actually sent by the client, which is understandable (links to Github, since hg.openjdk.org used to be unresponsive when writing this, and returned 504 for almost all requests): https://github.com/openjdk/jdk/blob/4fc6b0ffa4f771991a5ebd982b5133d2e364fdae/src/java.base/share/classes/sun/security/ssl/CertificateMessage.java#L416 But localCerts seems to be always set on the server side, with all the certificates being sent to the client: https://github.com/openjdk/jdk/blob/4fc6b0ffa4f771991a5ebd982b5133d2e364fdae/src/java.base/share/classes/sun/security/ssl/CertificateMessage.java#L265 This looks like an issue on the JDK side: there are no reasons why server certificates needs to be saved into the session on the server, as they are readily available on the server. Further, relevant saving code seems to be commented as "Client identity", which suggests that these might be saved unintentionally with an assumption that the code is only used on the client (OTOH, I don't see why the client needs its own certificates in the session as well). Do you have an affected JVM backend on hand to confirm it's indeed the case? I tend to think this needs to be reported to JDK developers. Their current code results in sending server certificate chain to the client at least two times (once in the handshake itself, and at least once in the ticket; not to mention that there can be more than one ticket in TLSv1.3), and they might reconsider doing this. (Funny enough, they seems to be using cache to deserialize certificates from such tickets, see https://bugs.openjdk.org/browse/JDK-8286433 for details.) Meanwhile, we can consider implementing a workaround on our side (that is, raising the limit, though I don't think there should be separate limits; also, I'm somewhat concerned about using 8k buffers on stack, we currently don't use anything larger than 4k) or instead focus on providing some guidance to users of affected JVM backends (I guess switching off tickets and/or TLSv1.3 should be enough in most cases). -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello!
On Wed, Jan 03, 2024 at 11:57:57PM +, Ben Kallus wrote:
> > Still, general style guidelines suggests that the code shouldn't
> > be written this way, and the only reason for j++ in the line in
> > question is that it mimics corresponding IPv4 code.
>
> > It's not "just happens".
>
> The point I'm trying to make is that ensuring correctness with
> function-like macros is difficult, both because of operator precedence
> and argument reevaluation. Expecting contributors to read the
> definitions of every macro they use becomes more and more cumbersome
> as the codebase expands, especially when some symbols are variably
> macros or functions depending on the state of (even infrequently-used)
> compile-time constants.
Sure, and hence the style.
> All that said, upon further reflection, I think the UB issue is best
> solved outside of ngx_strcpy, where the overhead of an extra check may
> have a performance impact. The following patch is sufficient to
> silence UBSan in my configuration:
I've already pointed you to the Vladimir's patch which contains at
least a dozen of places where the same "UB issue" is reported when
running nginx tests with UBSan. This demonstrates that your patch
is clearly insufficient. Further, Vladimir's patch is clearly
insufficient too, as shown for the another patch in the same
patch series.
If we want to follow this approach, we need some way to trace
places where zero length memcpy() can occur. My best guess is
that the only way is to look through all ngx_cpymem() /
ngx_memcpy() / ngx_memmove() / ngx_movemem() calls, as nginx
routinely uses { 0, NULL } as an empty string. Given that there
are 600+ of such calls in the codebase, and at least some need
serious audit to find out if { 0, NULL } can appear in the call,
this is going to be a huge work. And, given that the only
expected effect is theoretical correctness of the code, I doubt it
worth the effort, especially given that the end result will likely
reduce readability of the code.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello! On Fri, Dec 29, 2023 at 04:50:36PM +, Ben Kallus wrote: > > Still, -O0 is often used at least during development, and it might > > be unreasonable to introduce extra function calls in basic > > primitives. > > I don't think this is a major cause for concern. It is perfectly > reasonable for ngx_memcpy be a wrapper function around memcpy; I think > most people would assume that from the name. In fact, it's *already* > implemented as a function when NGX_MEMCPY_LIMIT is defined. The NGX_MEMCPY_LIMIT is a very specific debugging define, which implies additional overhead for memcpy calls. And it's certainly not an excuse for changing all the macro definitions to function calls. Whether or not ngx_memcpy() can be implemented as a wrapper function is a separate question. > > Further, nginx generally supports all available platforms > > reasonably compatible with POSIX and C89. This implies that > > inline might be not available. > > On such platforms, moving ngx_memcpy to a function may introduce some > performance overhead. The question is whether slightly better > performance on obscure systems is worth the mental overhead of working > with function-like macros. As said earlier in the thread, while some might prefer other approaches, function-like macros are used everywhere in nginx. > > Sure (but see above about performance overhead; and another > > question is if it needs to be solved, or following existing style > > is enough to never see the issue). > > A little extra performance overhead on C89 systems, or builds with -O0 > is a very small price to pay. ngx_resolver.c:4283 contains direct > evidence that function-like macros incur mental overhead: > ``` > ngx_memcpy(sin6->sin6_addr.s6_addr, addr6[j++].s6_addr, 16); > ``` > Here we have an expression with a side-effect being passed into a > function-like macro. As luck would have it, the second argument to > ngx_memcpy is evaluated only once, so this is coincidentally okay. > This particular landmine has been armed for a decade (see > https://github.com/nginx/nginx/blob/769eded73267274e018f460dd76b417538aa5934/src/core/ngx_resolver.c#L2902). > Thus, the existing style guidelines are not enough to prevent issues > with function-like macros from arising in nginx. Inline functions > solve this problem near-optimally. The mentioned call specifically assumes that the second argument is only evaluated once, and it was certainly checked that it is only evaluated once when the call was written. That is, there is no issue here. Still, general style guidelines suggests that the code shouldn't be written this way, and the only reason for j++ in the line in question is that it mimics corresponding IPv4 code. > > good luck with doing something like "ngx_max(foo & 0xff, bar)". > > Nginx is littered with uses of expressions in ngx_max and ngx_min, it > just happens that none of those expressions use operators of lower > precedence than < and >. This seems like an invitation for human > error. It's not "just happens". > Thus, I argue that the best solution to the memcpy-from-NULL problem > is to replace ngx_memcpy and ngx_cpymem with inline functions with the > appropriate checks for n==0. Going forward, it's probably smart to > consider transitioning away from function-like macros more generally. As I said earlier in this thread, I'm not exactly against using inline functions here, and I think a solution with inline functions can be accepted provided it is demonstrated that it introduces no measurable performance degradation. Still, I'm very sceptical about the idea of "transitioning away from function-like macros". -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 00 of 12] HTTP/3 proxying to upstreams
Hello! On Thu, Dec 28, 2023 at 05:23:38PM +0300, Vladimir Homutov via nginx-devel wrote: > On Thu, Dec 28, 2023 at 04:31:41PM +0300, Maxim Dounin wrote: > > Hello! > > > > On Wed, Dec 27, 2023 at 04:17:38PM +0300, Vladimir Homutov via nginx-devel > > wrote: > > > > > On Wed, Dec 27, 2023 at 02:48:04PM +0300, Maxim Dounin wrote: > > > > Hello! > > > > > > > > On Mon, Dec 25, 2023 at 07:52:41PM +0300, Vladimir Homutov via > > > > nginx-devel wrote: > > > > > > > > > Hello, everyone, > > > > > > > > > > and Merry Christmas to all! > > > > > > > > > > I'm a developer of an nginx fork Angie. Recently we implemented > > > > > an HTTP/3 proxy support in our fork [1]. > > > > > > > > > > We'd like to contribute this functionality to nginx OSS community. > > > > > Hence here is a patch series backported from Angie to the current > > > > > head of nginx mainline branch (1.25.3) > > > > > > > > Thank you for the patches. > > > > > > > > Are there any expected benefits from HTTP/3 being used as a > > > > protocol to upstream servers? > > > > > > Personally, I don't see much. > > > > > > Probably, faster connection establishing to due 0RTT support (need to be > > > implemented) and better multiplexing (again, if implemented wisely). > > > I have made some simple benchmarks, and it looks more or less similar > > > to usual SSL connections. > > > > Thanks for the details. > > > > Multiplexing is available since introduction of the FastCGI > > protocol, yet to see it working in upstream connections. As for > > 0-RTT, using keepalive connections is probably more efficient > > anyway (and not really needed for upstream connections in most > > cases as well). > > With HTTP/3 and keepalive we can have just one quic "connection" per upstream > server (in extreme). We perform heavy handshake once, and leave it open. > Next we just create HTTP/3 streams to perform request. They can perfectly > run in parallel and use same quic connection. Probably, this is something > worth implementing, with limitations of course: we don't want to mix > requests from different (classes of) clients in same connection, we > don't want eternal life of such connection and we need means to control > level of such multiplexing. Multiplexing has various downsides: already mentioned security implications, issues with balancing requests between upstream entities not directly visible to the client (such as different worker processes), added complexity. And, as already mentioned, it is not something new in HTTP/3. [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 00 of 12] HTTP/3 proxying to upstreams
Hello! On Wed, Dec 27, 2023 at 04:17:38PM +0300, Vladimir Homutov via nginx-devel wrote: > On Wed, Dec 27, 2023 at 02:48:04PM +0300, Maxim Dounin wrote: > > Hello! > > > > On Mon, Dec 25, 2023 at 07:52:41PM +0300, Vladimir Homutov via nginx-devel > > wrote: > > > > > Hello, everyone, > > > > > > and Merry Christmas to all! > > > > > > I'm a developer of an nginx fork Angie. Recently we implemented > > > an HTTP/3 proxy support in our fork [1]. > > > > > > We'd like to contribute this functionality to nginx OSS community. > > > Hence here is a patch series backported from Angie to the current > > > head of nginx mainline branch (1.25.3) > > > > Thank you for the patches. > > > > Are there any expected benefits from HTTP/3 being used as a > > protocol to upstream servers? > > Personally, I don't see much. > > Probably, faster connection establishing to due 0RTT support (need to be > implemented) and better multiplexing (again, if implemented wisely). > I have made some simple benchmarks, and it looks more or less similar > to usual SSL connections. Thanks for the details. Multiplexing is available since introduction of the FastCGI protocol, yet to see it working in upstream connections. As for 0-RTT, using keepalive connections is probably more efficient anyway (and not really needed for upstream connections in most cases as well). > > > > [...] > > > > > Probably, the HTTP/3 proxy should be implemented in a separate module. > > > Currently it is a patch to the HTTP proxy module to minimize > > > boilerplate. > > > > Sure. I'm very much against the idea of mixing different upstream > > protocols in a single protocol module. > > noted. > > > (OTOH, there are some uncertain plans to make proxy module able to > > work with other protocols based on the scheme, such as in > > "proxy_pass fastcgi://127.0.0.1:9000;". This is mostly irrelevant > > though, and might never happen.) > > well, currently we have separate proxying modules that are similar enough to > think about merging them like suggested. Not sure if one big module with > methods will worth it, as semantics is slightly different. > > proxy modules are already addons on top of upstream module, which does > the heavy lifting. What requires improvement is probably the > configuration that makes user to remember many similar directives doing > the same thing but for different protocols. Yep, making things easier to configure (and modify, if something related to configuration directives is changed or additional protocol is added) is the main motivator. Still, there are indeed differences between protocol modules, and this makes single module inconvenient sometimes. As such, plans are uncertain (and the previous attempt to do this failed miserably). -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Re: Don't delete timer of write event when it's delayed.
Hello! On Wed, Dec 27, 2023 at 08:38:15PM +0800, Jiuzhou Cui wrote: > Thank you for your reply. > > Firstly, we meet the problem. And this patch works for me. > > My scenario is after send response body about 10-20MB, we just set: > 1. limit_rate = 1KB > 2. limit_rate_after = body_bytes_sent > 3. proxy_buffering = "on" （I think this is the key issue） > > At the request begining, we didn't set proxy_buffering = "on" and limit_rate. Sorry, not sure what you are trying to say. You modify r->limit_rate and r->limit_rate_after from your module after sending some parts of the response? This is not expected to work due to the mentioned design limitation of non-buffered proxying, and generally looks like a bug in your module, it shouldn't do this. Further, it is not possible to change upstream buffering after nginx started sending the response. It's a one-time choice, and modifications of r->upstream->buffering won't do anything (though also incorrect, as it's not something expected to be modified by modules). Or I understood something incorrectly? -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 00 of 12] HTTP/3 proxying to upstreams
Hello! On Mon, Dec 25, 2023 at 07:52:41PM +0300, Vladimir Homutov via nginx-devel wrote: > Hello, everyone, > > and Merry Christmas to all! > > I'm a developer of an nginx fork Angie. Recently we implemented > an HTTP/3 proxy support in our fork [1]. > > We'd like to contribute this functionality to nginx OSS community. > Hence here is a patch series backported from Angie to the current > head of nginx mainline branch (1.25.3) Thank you for the patches. Are there any expected benefits from HTTP/3 being used as a protocol to upstream servers? [...] > Probably, the HTTP/3 proxy should be implemented in a separate module. > Currently it is a patch to the HTTP proxy module to minimize boilerplate. Sure. I'm very much against the idea of mixing different upstream protocols in a single protocol module. (OTOH, there are some uncertain plans to make proxy module able to work with other protocols based on the scheme, such as in "proxy_pass fastcgi://127.0.0.1:9000;". This is mostly irrelevant though, and might never happen.) [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Don't delete timer of write event when it's delayed.
Hello!
On Wed, Dec 27, 2023 at 10:56:44AM +0800, Jiuzhou Cui wrote:
> Hello!
>
>
> # HG changeset patch
> # User Jiuzhou Cui
> # Date 1703645578 -28800
> # Wed Dec 27 10:52:58 2023 +0800
> # Node ID 474ae07e47272e435d81c0ca9e4867aae35c30ab
> # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07
> Don't delete timer of write event when it's delayed.
>
>
> This will make download speed alway zero when limit_rate in body filter.
>
>
> diff -r ee40e2b1d083 -r 474ae07e4727 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c Mon Dec 25 21:15:48 2023 +0400
> +++ b/src/http/ngx_http_upstream.c Wed Dec 27 10:52:58 2023 +0800
> @@ -3787,11 +3787,13 @@
> }
> }
>
>
> -if (downstream->write->active && !downstream->write->ready) {
> -ngx_add_timer(downstream->write, clcf->send_timeout);
> -
> -} else if (downstream->write->timer_set) {
> -ngx_del_timer(downstream->write);
> +if (!downstream->write->delayed) {
> +if (downstream->write->active && !downstream->write->ready) {
> +ngx_add_timer(downstream->write, clcf->send_timeout);
> +
> +} else if (downstream->write->timer_set) {
> +ngx_del_timer(downstream->write);
> +}
> }
>
>
> if (upstream->read->eof || upstream->read->error) {
Thank you for the patch.
You are patching the
ngx_http_upstream_process_non_buffered_request() function, which
is, as can be correctly concluded from the function name, is used
for non-buffered proxying. Non-buffered proxying is specifically
designed to return responses as long as they are available, and is
not compatible with limit_rate. Moreover, limit_rate is
explicitly disabled in the ngx_http_upstream_send_response()
function when the relevant handers are set:
u->read_event_handler = ngx_http_upstream_process_non_buffered_upstream;
r->write_event_handler =
ngx_http_upstream_process_non_buffered_downstream;
r->limit_rate = 0;
r->limit_rate_set = 1;
(https://hg.nginx.org/nginx/file/release-1.25.3/src/http/ngx_http_upstream.c#l3092)
As such, the issue you are trying to fix is not expected to
appear.
Could you please clarify the configuration you are seeing the
issue with, and steps to reproduce the issue?
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6
Hello! On Fri, Dec 22, 2023 at 05:53:30PM +0400, Sergey Kandaurov wrote: > > > On 21 Dec 2023, at 02:40, Maxim Dounin wrote: > > > > Hello! > > > > On Tue, Dec 19, 2023 at 10:44:00PM +0400, Sergey Kandaurov wrote: > > > >> > >>> On 19 Dec 2023, at 12:58, Maxim Dounin wrote: > >>> > >>> Hello! > >>> > >>> On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote: > >>> > >>>>> On 24 Nov 2023, at 00:29, Ilya Shipitsin wrote: > >>>>> > >>>>> # HG changeset patch > >>>>> # User Ilya Shipitsin > >>>>> # Date 1700769135 -3600 > >>>>> # Thu Nov 23 20:52:15 2023 +0100 > >>>>> # Node ID 2001e73ce136d5bfc9bde27d338865b14b8ad436 > >>>>> # Parent 7ec761f0365f418511e30b82e9adf80bc56681df > >>>>> ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6 > >>>> > >>>> style: SSL prefix should be uppercase. > >>>> > >>>>> > >>>>> diff -r 7ec761f0365f -r 2001e73ce136 > >>>>> src/event/ngx_event_openssl_stapling.c > >>>>> --- a/src/event/ngx_event_openssl_stapling.cThu Oct 26 23:35:09 > >>>>> 2023 +0300 > >>>>> +++ b/src/event/ngx_event_openssl_stapling.cThu Nov 23 20:52:15 > >>>>> 2023 +0100 > >>>>> @@ -893,7 +893,8 @@ > >>>>> ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > >>>>> ocsp->conf = ocf; > >>>>> > >>>>> -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > >>>>> LIBRESSL_VERSION_NUMBER) > >>>>> +/* minimum OpenSSL 1.1.1 & LibreSSL 3.3.6 */ > >>>>> +#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > >>>>> LIBRESSL_VERSION_NUMBER) || (defined(LIBRESSL_VERSION_NUMBER) && > >>>>> (LIBRESSL_VERSION_NUMBER >= 0x3030600L)) > >>>>> > >>>>> ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > >>>>> > >>>> > >>>> Testing "defined(LIBRESSL_VERSION_NUMBER)" is superfluous. > >>>> The macro test suffers from a very long line. > >>>> > >>>> The correct version test seems to be against LibreSSL 3.5.0, see > >>>> https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.5.0-relnotes.txt > >>>> > >>>> So, the resulting change would be as follows: > >>>> > >>>> diff --git a/src/event/ngx_event_openssl_stapling.c > >>>> b/src/event/ngx_event_openssl_stapling.c > >>>> --- a/src/event/ngx_event_openssl_stapling.c > >>>> +++ b/src/event/ngx_event_openssl_stapling.c > >>>> @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > >>>>ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > >>>>ocsp->conf = ocf; > >>>> > >>>> -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > >>>> LIBRESSL_VERSION_NUMBER) > >>>> +#if (OPENSSL_VERSION_NUMBER >= 0x1010L \ > >>>> + && !defined LIBRESSL_VERSION_NUMBER) \ > >>>> +|| LIBRESSL_VERSION_NUMBER >= 0x305fL > >>> > >>> Rather, > >>> > >>> +#if (OPENSSL_VERSION_NUMBER >= 0x1010L > >>> \ > >>> + && (!defined LIBRESSL_VERSION_NUMBER > >>> \ > >>> + || LIBRESSL_VERSION_NUMBER >= 0x305fL)) > >>> > >> > >> Agree. For the sake of completeness: > >> > >> # HG changeset patch > >> # User Sergey Kandaurov > >> # Date 1703004490 -14400 > >> # Tue Dec 19 20:48:10 2023 +0400 > >> # Node ID 267cee796462f4f6bacf825c8fd24d13845d36f4 > >> # Parent 7a6d52990f2e2d88460a3dc6cc84aac89b7329ea > >> SSL: using SSL_get0_verified_chain() with LibreSSL 3.5.0+. > >> > >> Prodded by Ilya Shipitsin. > >> > >> diff --git a/src/event/ngx_event_openssl_stapling.c > >> b/src/event/ngx_event_openssl_stapling.c > >> --- a/src/event/ngx_event_openssl_stapling.c > >> +++ b/src/event/ngx_event_openssl_stapling.c > >> @@ -893,7 +893,9 @@ ngx_ssl_ocsp_va
Re: [PATCH] SSL: raised limit for upstream session size
Hello! On Fri, Dec 22, 2023 at 06:28:34PM +0400, Sergey Kandaurov wrote: > # HG changeset patch > # User Sergey Kandaurov > # Date 1703255284 -14400 > # Fri Dec 22 18:28:04 2023 +0400 > # Node ID a463fb67e143c051fd373d1df94e5813a37d5cea > # Parent 44266e0651c44f530c4aa66e68c1b9464a9acee7 > SSL: raised limit for upstream session size. > > Unlike shared session cache used to store multiple client SSL sessions and > which may be per a single SSL connection, sessions saved from upstream are > per upstream server peer, so there is no such multiplier effect, but they > may be of noticeably larger size due to session tickets being used. > > It was observed that session tickets sent from JVM backends may result in > a decoded session size nearly the previous maximum session size limit of > 4096 or slightly beyond. Raising the limit allows to save such sessions. Session tickets are not expected to be larger than sessions itself, except by several bytes used for key identification and encryption overhead. I see no reasons why the limit should be different in different places. And 4096 for an SSL session looks a lot. The only justification I can assume here is an SSL session with the client certificate (or even certificate chain) being saved into the session. It might worth looking into what actually happens here. [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 2 of 2] Win32: extended ngx_random range to 0x7fffffff
Hello! On Fri, Dec 22, 2023 at 05:52:30PM +0400, Sergey Kandaurov wrote: > On Thu, Dec 21, 2023 at 07:14:40PM +0300, Maxim Dounin wrote: > > Hello! > > > > On Thu, Dec 21, 2023 at 05:37:02PM +0400, Sergey Kandaurov wrote: > > > > > > On 16 Dec 2023, at 06:57, Maxim Dounin wrote: > > > > > > > > Hello! > > > > > > > > On Sat, Dec 09, 2023 at 08:42:11AM +, J Carter wrote: > > > > > > > >> On Sat, 09 Dec 2023 07:46:10 + > > > >> J Carter wrote: > > > >> > > > >>> # HG changeset patch > > > >>> # User J Carter > > > >>> # Date 1702101635 0 > > > >>> # Sat Dec 09 06:00:35 2023 + > > > >>> # Node ID 1a77698f82d2580aa8b8f62ce89b4dbb6d678c5d > > > >>> # Parent d9275e982a7188a1ea7855295ffa93362ea9830f > > > >>> Win32: extended ngx_random range to 0x7fff > > > >>> > > > >>> rand() is used on win32. RAND_MAX is implementation defined. win32's > > > >>> is > > > >>> 0x7fff. > > > >>> > > > >>> Existing uses of ngx_random rely upon 0x7fff range provided by > > > >>> posix implementations of random(). > > > >>> > > > >>> diff -r d9275e982a71 -r 1a77698f82d2 src/os/win32/ngx_win32_config.h > > > >>> --- a/src/os/win32/ngx_win32_config.h Sat Dec 09 05:09:07 2023 + > > > >>> +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 06:00:35 2023 + > > > >>> @@ -280,7 +280,9 @@ > > > >>> > > > >>> #define NGX_HAVE_GETADDRINFO 1 > > > >>> > > > >>> -#define ngx_random rand > > > >>> +#define ngx_random > > > >>> \ > > > >>> +((rand() << 16) | (rand() << 1) | (rand() >> 14)) > > > >>> + > > > >>> #define ngx_debug_init() > > > >>> > > > >>> > > > >> > > > >> ^ my mistake - copying error.. > > > >> > > > >> # HG changeset patch > > > >> # User J Carter > > > >> # Date 1702111094 0 > > > >> # Sat Dec 09 08:38:14 2023 + > > > >> # Node ID 10ef59a412a330872fc6d46de64f42e32e997b3a > > > >> # Parent d9275e982a7188a1ea7855295ffa93362ea9830f > > > >> Win32: extended ngx_random range to 0x7fff > > > > > > > > Nitpicking: > > > > > > > > Win32: extended ngx_random() range to 0x7fff. > > > > > > > >> > > > >> rand() is used on win32. RAND_MAX is implementation defined. win32's is > > > >> 0x7fff. > > > >> > > > >> Existing uses of ngx_random rely upon 0x7fff range provided by > > > >> posix implementations of random(). > > > > > > > > Thanks for catching this. > > > > > > > > As far as I can see, the only module which actually relies on the > > > > range is the random index module. Relying on the ngx_random() > > > > range generally looks wrong to me, and we might want to change the > > > > code to don't. OTOH, it's the only way to get a completely > > > > uniform distribution, and that's what the module tries to do. As > > > > such, it might be good enough to preserve it as is, at least till > > > > further changes to ngx_random(). > > > > > > > > Either way, wider range for ngx_random() should be beneficial in > > > > other places. > > > > > > > >> > > > >> diff -r d9275e982a71 -r 10ef59a412a3 src/os/win32/ngx_win32_config.h > > > >> --- a/src/os/win32/ngx_win32_config.h Sat Dec 09 05:09:07 2023 + > > > >> +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 08:38:14 2023 + > > > >> @@ -280,7 +280,9 @@ > > > >> > > > >> #define NGX_HAVE_GETADDRINFO 1 > > > >> > > > >> -#define ngx_random rand > > > >> +#define ngx_random() > > > >>\ > > > > > > > > Nitpicking: the "\" character should be a
Re: [PATCH 2 of 2] Win32: extended ngx_random range to 0x7fffffff
Hello! On Thu, Dec 21, 2023 at 05:37:02PM +0400, Sergey Kandaurov wrote: > > On 16 Dec 2023, at 06:57, Maxim Dounin wrote: > > > > Hello! > > > > On Sat, Dec 09, 2023 at 08:42:11AM +, J Carter wrote: > > > >> On Sat, 09 Dec 2023 07:46:10 + > >> J Carter wrote: > >> > >>> # HG changeset patch > >>> # User J Carter > >>> # Date 1702101635 0 > >>> # Sat Dec 09 06:00:35 2023 + > >>> # Node ID 1a77698f82d2580aa8b8f62ce89b4dbb6d678c5d > >>> # Parent d9275e982a7188a1ea7855295ffa93362ea9830f > >>> Win32: extended ngx_random range to 0x7fff > >>> > >>> rand() is used on win32. RAND_MAX is implementation defined. win32's is > >>> 0x7fff. > >>> > >>> Existing uses of ngx_random rely upon 0x7fff range provided by > >>> posix implementations of random(). > >>> > >>> diff -r d9275e982a71 -r 1a77698f82d2 src/os/win32/ngx_win32_config.h > >>> --- a/src/os/win32/ngx_win32_config.h Sat Dec 09 05:09:07 2023 + > >>> +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 06:00:35 2023 + > >>> @@ -280,7 +280,9 @@ > >>> > >>> #define NGX_HAVE_GETADDRINFO 1 > >>> > >>> -#define ngx_random rand > >>> +#define ngx_random > >>> \ > >>> +((rand() << 16) | (rand() << 1) | (rand() >> 14)) > >>> + > >>> #define ngx_debug_init() > >>> > >>> > >> > >> ^ my mistake - copying error.. > >> > >> # HG changeset patch > >> # User J Carter > >> # Date 1702111094 0 > >> # Sat Dec 09 08:38:14 2023 + > >> # Node ID 10ef59a412a330872fc6d46de64f42e32e997b3a > >> # Parent d9275e982a7188a1ea7855295ffa93362ea9830f > >> Win32: extended ngx_random range to 0x7fff > > > > Nitpicking: > > > > Win32: extended ngx_random() range to 0x7fff. > > > >> > >> rand() is used on win32. RAND_MAX is implementation defined. win32's is > >> 0x7fff. > >> > >> Existing uses of ngx_random rely upon 0x7fff range provided by > >> posix implementations of random(). > > > > Thanks for catching this. > > > > As far as I can see, the only module which actually relies on the > > range is the random index module. Relying on the ngx_random() > > range generally looks wrong to me, and we might want to change the > > code to don't. OTOH, it's the only way to get a completely > > uniform distribution, and that's what the module tries to do. As > > such, it might be good enough to preserve it as is, at least till > > further changes to ngx_random(). > > > > Either way, wider range for ngx_random() should be beneficial in > > other places. > > > >> > >> diff -r d9275e982a71 -r 10ef59a412a3 src/os/win32/ngx_win32_config.h > >> --- a/src/os/win32/ngx_win32_config.h Sat Dec 09 05:09:07 2023 + > >> +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 08:38:14 2023 + > >> @@ -280,7 +280,9 @@ > >> > >> #define NGX_HAVE_GETADDRINFO 1 > >> > >> -#define ngx_random rand > >> +#define ngx_random() > >>\ > > > > Nitpicking: the "\" character should be at the 79th column (in > > some files at 78th). This ensures that a diff won't wrap on a > > standard 80-column terminal. > > > >> +((rand() << 16) | (rand() << 1) | (rand() >> 14)) > >> + > >> #define ngx_debug_init() > > > > Using "|" for random numbers looks utterly wrong to me, even if > > ORed values are guaranteed to not interfere. > > > > I would rather use "^", and avoid dependency on the particular > > value of RAND_MAX (other than POSIX-required minimum of 32767) by > > using something like > > > > 0x7fff & ((rand() << 16) ^ (rand() << 8) ^ rand()) > > > > with proper typecasts. > > > > Something like this should work: > > > > diff --git a/src/os/win32/ngx_win32_config.h > > b/src/os/win32/ngx_win32_config.h > > --- a/src/os/win32/ngx_win32_config.h > > +++ b/src/os/win32/ngx_win32_config.h > > @@ -280,7 +280,11 @@ typedef int
Re: processing a request without body
Hello! On Tue, Dec 19, 2023 at 10:11:04PM +0800, Muhammad Nuzaihan wrote: > Thanks Maxim, Vasility, > > The problem i was going to solve is to i needed to run my specific > function that takes the data of request URL path, Headers and request > body and determine and validate that all that data is correct before > sending upstream, or else i would deny the request with 4xx code > errors. > > Handlers can only handle (from what i know) URL path and headers. > > Request body requires a request chain (ngx_chain_t)) to piece out the > request body and handlers doesn't seem to have t ngx_chain_t unlike > request body filters. > > Or maybe i am wrong in this case? It looks like you are trying to do something which simply cannot be done. For example, consider a configuration with "proxy_request_buffering off;" - in such a configuration request body is being read _after_ the request is passed to the upstream server, and you simply cannot validate request body before passing request headers to the upstream server. As long as you have to examine both request body and request headers, I think there can be two possible solutions: 1. Install a phase handler, in which read the request body yourself, and check both request headers and request body once it's read. See the mirror module as an example on how to read the body in a phase handler and properly resume processing after it. This will break proxying without request buffering, though might be good enough for your particular task. 2. Install a phase handler to check request headers, and a request body filter to check the request body. Do checking in both places, and abort request processing when you see that data aren't correct. This will work with proxying without request buffering, but will be generally more complex to implement. And, obviously, this in case of proxying without request buffering this won't let you to validate request body before the request headers are sent to upstream server. Hope this helps. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello! On Sat, Dec 16, 2023 at 04:26:37PM -0500, Ben Kallus wrote: > > In general macro definitions in nginx are used everywhere for > > efficiency reasons > > Clang inlines short functions with -O1, and GCC does so with -O2 or > -O1 -finline-small-functions. Are there any platforms that Nginx needs > to support for which short function inlining isn't sufficient to solve > this issue? In nginx, functions expected to be inlined are marked with "ngx_inline", which normally resolves to "inline" (on unix) or "__inline" (on win32). As such, modern versions of both clang and gcc will inline corresponding functions unless optimization is disabled. Still, -O0 is often used at least during development, and it might be unreasonable to introduce extra function calls in basic primitives. Further, nginx generally supports all available platforms reasonably compatible with POSIX and C89. This implies that inline might be not available. > > While some might prefer other approaches, writing code like > > "ngx_memcpy(dst, src, ++len)" in nginx is just wrong, and > > shouldn't be trusted to work, much like it won't work with > > "ngx_cpymem(dst, src, ++len)". > > It is indeed wrong to use an expression with a side-effect in an > argument to cpymem, but it's also not very obvious that it's wrong. An > inlined function solves the argument reevaluation issue without > performance overhead. Sure (but see above about performance overhead; and another question is if it needs to be solved, or following existing style is enough to never see the issue). The point is: in nginx, it's anyway wrong to use arguments with side effects. And even expressions without side effects might won't work. While many macro definitions were adjusted to accept expressions instead of the bare variables (see 2f9214713666 and https://mailman.nginx.org/pipermail/nginx-devel/2020-July/013354.html for an example), some still don't or can be picky. For example, good luck with doing something like "ngx_max(foo & 0xff, bar)". As such, it's certainly not an argument against using checks in macro definitions in the particular patch. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6
Hello! On Tue, Dec 19, 2023 at 10:44:00PM +0400, Sergey Kandaurov wrote: > > > On 19 Dec 2023, at 12:58, Maxim Dounin wrote: > > > > Hello! > > > > On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote: > > > >>> On 24 Nov 2023, at 00:29, Ilya Shipitsin wrote: > >>> > >>> # HG changeset patch > >>> # User Ilya Shipitsin > >>> # Date 1700769135 -3600 > >>> # Thu Nov 23 20:52:15 2023 +0100 > >>> # Node ID 2001e73ce136d5bfc9bde27d338865b14b8ad436 > >>> # Parent 7ec761f0365f418511e30b82e9adf80bc56681df > >>> ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6 > >> > >> style: SSL prefix should be uppercase. > >> > >>> > >>> diff -r 7ec761f0365f -r 2001e73ce136 > >>> src/event/ngx_event_openssl_stapling.c > >>> --- a/src/event/ngx_event_openssl_stapling.c Thu Oct 26 23:35:09 > >>> 2023 +0300 > >>> +++ b/src/event/ngx_event_openssl_stapling.c Thu Nov 23 20:52:15 > >>> 2023 +0100 > >>> @@ -893,7 +893,8 @@ > >>>ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > >>>ocsp->conf = ocf; > >>> > >>> -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > >>> LIBRESSL_VERSION_NUMBER) > >>> +/* minimum OpenSSL 1.1.1 & LibreSSL 3.3.6 */ > >>> +#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > >>> LIBRESSL_VERSION_NUMBER) || (defined(LIBRESSL_VERSION_NUMBER) && > >>> (LIBRESSL_VERSION_NUMBER >= 0x3030600L)) > >>> > >>>ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > >>> > >> > >> Testing "defined(LIBRESSL_VERSION_NUMBER)" is superfluous. > >> The macro test suffers from a very long line. > >> > >> The correct version test seems to be against LibreSSL 3.5.0, see > >> https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.5.0-relnotes.txt > >> > >> So, the resulting change would be as follows: > >> > >> diff --git a/src/event/ngx_event_openssl_stapling.c > >> b/src/event/ngx_event_openssl_stapling.c > >> --- a/src/event/ngx_event_openssl_stapling.c > >> +++ b/src/event/ngx_event_openssl_stapling.c > >> @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > >> ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > >> ocsp->conf = ocf; > >> > >> -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > >> LIBRESSL_VERSION_NUMBER) > >> +#if (OPENSSL_VERSION_NUMBER >= 0x1010L \ > >> + && !defined LIBRESSL_VERSION_NUMBER) \ > >> +|| LIBRESSL_VERSION_NUMBER >= 0x305fL > > > > Rather, > > > > +#if (OPENSSL_VERSION_NUMBER >= 0x1010L > >\ > > + && (!defined LIBRESSL_VERSION_NUMBER > >\ > > + || LIBRESSL_VERSION_NUMBER >= 0x305fL)) > > > > Agree. For the sake of completeness: > > # HG changeset patch > # User Sergey Kandaurov > # Date 1703004490 -14400 > # Tue Dec 19 20:48:10 2023 +0400 > # Node ID 267cee796462f4f6bacf825c8fd24d13845d36f4 > # Parent 7a6d52990f2e2d88460a3dc6cc84aac89b7329ea > SSL: using SSL_get0_verified_chain() with LibreSSL 3.5.0+. > > Prodded by Ilya Shipitsin. > > diff --git a/src/event/ngx_event_openssl_stapling.c > b/src/event/ngx_event_openssl_stapling.c > --- a/src/event/ngx_event_openssl_stapling.c > +++ b/src/event/ngx_event_openssl_stapling.c > @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > ocsp->conf = ocf; > > -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > LIBRESSL_VERSION_NUMBER) > +#if (OPENSSL_VERSION_NUMBER >= 0x1010L > \ > + && (!defined LIBRESSL_VERSION_NUMBER > \ > + || LIBRESSL_VERSION_NUMBER >= 0x305fL)) > > ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > > > >> > >> ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > >> > >> > >> On the other hand, I don't like the resulting style mudness. > >> It may have sense just to drop old LibreSSL versions support: > >> maintaining one or two m
Re: [PATCH] ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6
Hello! On Tue, Dec 19, 2023 at 12:16:58PM +0100, Илья Шипицин wrote: > вт, 19 дек. 2023 г. в 09:58, Maxim Dounin : > > > Hello! > > > > On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote: > > > > > > On 24 Nov 2023, at 00:29, Ilya Shipitsin wrote: > > > > > > > > # HG changeset patch > > > > # User Ilya Shipitsin > > > > # Date 1700769135 -3600 > > > > # Thu Nov 23 20:52:15 2023 +0100 > > > > # Node ID 2001e73ce136d5bfc9bde27d338865b14b8ad436 > > > > # Parent 7ec761f0365f418511e30b82e9adf80bc56681df > > > > ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6 > > > > > > style: SSL prefix should be uppercase. > > > > > > > > > > > diff -r 7ec761f0365f -r 2001e73ce136 > > src/event/ngx_event_openssl_stapling.c > > > > --- a/src/event/ngx_event_openssl_stapling.cThu Oct 26 > > 23:35:09 2023 +0300 > > > > +++ b/src/event/ngx_event_openssl_stapling.cThu Nov 23 > > 20:52:15 2023 +0100 > > > > @@ -893,7 +893,8 @@ > > > > ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > > > > ocsp->conf = ocf; > > > > > > > > -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > > LIBRESSL_VERSION_NUMBER) > > > > +/* minimum OpenSSL 1.1.1 & LibreSSL 3.3.6 */ > > > > +#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > > LIBRESSL_VERSION_NUMBER) || (defined(LIBRESSL_VERSION_NUMBER) && > > (LIBRESSL_VERSION_NUMBER >= 0x3030600L)) > > > > > > > > ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > > > > > > > > > > Testing "defined(LIBRESSL_VERSION_NUMBER)" is superfluous. > > > The macro test suffers from a very long line. > > > > > > The correct version test seems to be against LibreSSL 3.5.0, see > > > https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.5.0-relnotes.txt > > > > > > So, the resulting change would be as follows: > > > > > > diff --git a/src/event/ngx_event_openssl_stapling.c > > b/src/event/ngx_event_openssl_stapling.c > > > --- a/src/event/ngx_event_openssl_stapling.c > > > +++ b/src/event/ngx_event_openssl_stapling.c > > > @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > > > ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > > > ocsp->conf = ocf; > > > > > > -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > > LIBRESSL_VERSION_NUMBER) > > > +#if (OPENSSL_VERSION_NUMBER >= 0x1010L \ > > > + && !defined LIBRESSL_VERSION_NUMBER) \ > > > +|| LIBRESSL_VERSION_NUMBER >= 0x305fL > > > > Rather, > > > > +#if (OPENSSL_VERSION_NUMBER >= 0x1010L > > \ > > + && (!defined LIBRESSL_VERSION_NUMBER > > \ > > + || LIBRESSL_VERSION_NUMBER >= 0x305fL)) > > > > > > > > ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > > > > > > > > > On the other hand, I don't like the resulting style mudness. > > > It may have sense just to drop old LibreSSL versions support: > > > maintaining one or two most recent stable branches should be enough. > > > > +1 on this. > > > > if we want to keep code clean, we can move "if LibreSSL >= 3.7" to > "configure" level I wouldn't expect such a change to be accepted. We generally don't try to test various SSL library features in configure - there are a lot of things to check, and testing appropriate defines (or version numbers, if no defines are available) is believed to work much better. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Meaning of proxy_ignore_client_abort
Hello! On Tue, Dec 19, 2023 at 04:02:46PM +0100, Jan Prachař wrote: > Hello, > > I have proxy module configured with proxy_ignore_client_abort on; but > connection to > upstream is still being closed. This is in debug log: > > writev() failed (32: Broken pipe) while sending to client, ... > http write filter > http copy filter: -1 ... > pipe read upstream: 0 > pipe buf free s:0 t:1 f:0 55DEBEF95EC0, pos 55DEBEF95EC0, size: 473 > file: 0, > size: 0 > pipe buf free s:0 t:1 f:0 55DEBEF91EB0, pos 55DEBEF91EB0, size: 0 > file: 0, size: > 0 > pipe length: 22594336 > event timer: 23, old: 15583745, new: 15583837 > http upstream downstream error > finalize http upstream request: -1 > finalize http proxy request > close http upstream connection: 23 > > It seems, that check if ignore_client_abort is on, is missing here: > https://trac.nginx.org/nginx/browser/nginx/src/http/ngx_http_upstream.c#L4223 > > Or is there any reason why the connection is closed regardless the > ignore_client_abort? When an error while sending to the client occurs, like in the log you've provided, the connection is closed regardless of the "proxy_ignore_client_abort" directive. The directive only affects nginx behaviour when nginx is waiting for a response from the upstream server: with "proxy_ignore_client_abort on;" nginx will not try to detect if the client already closed the connection and close the upstream connection accordingly. When the response is being sent, the upstream server is expected to be smart enough to recognize that the connection was closed. Note that the docs say (http://nginx.org/r/proxy_ignore_client_abort): : Determines whether the connection with a proxied server should : be closed when a client closes the connection without waiting for : a response. While it probably can be improved, it explicitly says "without waiting for a response", and nothing about "when reading a response". -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6
Hello! On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote: > > On 24 Nov 2023, at 00:29, Ilya Shipitsin wrote: > > > > # HG changeset patch > > # User Ilya Shipitsin > > # Date 1700769135 -3600 > > # Thu Nov 23 20:52:15 2023 +0100 > > # Node ID 2001e73ce136d5bfc9bde27d338865b14b8ad436 > > # Parent 7ec761f0365f418511e30b82e9adf80bc56681df > > ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6 > > style: SSL prefix should be uppercase. > > > > > diff -r 7ec761f0365f -r 2001e73ce136 src/event/ngx_event_openssl_stapling.c > > --- a/src/event/ngx_event_openssl_stapling.cThu Oct 26 23:35:09 > > 2023 +0300 > > +++ b/src/event/ngx_event_openssl_stapling.cThu Nov 23 20:52:15 > > 2023 +0100 > > @@ -893,7 +893,8 @@ > > ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > > ocsp->conf = ocf; > > > > -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > > LIBRESSL_VERSION_NUMBER) > > +/* minimum OpenSSL 1.1.1 & LibreSSL 3.3.6 */ > > +#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > > LIBRESSL_VERSION_NUMBER) || (defined(LIBRESSL_VERSION_NUMBER) && > > (LIBRESSL_VERSION_NUMBER >= 0x3030600L)) > > > > ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > > > > Testing "defined(LIBRESSL_VERSION_NUMBER)" is superfluous. > The macro test suffers from a very long line. > > The correct version test seems to be against LibreSSL 3.5.0, see > https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.5.0-relnotes.txt > > So, the resulting change would be as follows: > > diff --git a/src/event/ngx_event_openssl_stapling.c > b/src/event/ngx_event_openssl_stapling.c > --- a/src/event/ngx_event_openssl_stapling.c > +++ b/src/event/ngx_event_openssl_stapling.c > @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > ocsp->conf = ocf; > > -#if (OPENSSL_VERSION_NUMBER >= 0x1010L && !defined > LIBRESSL_VERSION_NUMBER) > +#if (OPENSSL_VERSION_NUMBER >= 0x1010L \ > + && !defined LIBRESSL_VERSION_NUMBER) \ > +|| LIBRESSL_VERSION_NUMBER >= 0x305fL Rather, +#if (OPENSSL_VERSION_NUMBER >= 0x1010L\ + && (!defined LIBRESSL_VERSION_NUMBER \ + || LIBRESSL_VERSION_NUMBER >= 0x305fL)) > > ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > > > On the other hand, I don't like the resulting style mudness. > It may have sense just to drop old LibreSSL versions support: > maintaining one or two most recent stable branches should be enough. +1 on this. We certainly don't want to maintain support for ancient LibreSSL versions. IMO, just two branches is more than enough (and this is what I use in my testing, which usually means latest development release and latest stable release). At most, this probably can be three branches - which seems to match LibreSSL support policy, https://www.libressl.org/releases.html: : LibreSSL transitions to a new stable release branch every 6 months : in coordination with the OpenBSD development schedule. LibreSSL : stable branches are updated for 1 year after their corresponding : OpenBSD branch is tagged for release. See below for the current : stable release branches. In either case, LibreSSL versions below 3.5.0 are already not supported. If I understand correctly, right now the oldest supported branch is 3.7.x. > But anyway, I don't see an obvious win over the existing code: > the certificate chain is reconstructed if SSL_get0_verified_chain() > is (detected to be) not present, which should be fine in most cases. > > That said, it doesn't seem to deserve introducing 3-line macro test, > or (see OTOH note) breaking old LibreSSL support for no apparent reason. Reconstruction of the chain implies verification of signatures along the chain and can be costly. As such, it certainly would be better to use SSL_get0_verified_chain() as long as it is available. Also, removing the "!defined LIBRESSL_VERSION_NUMBER" check might be seen as positive even without any additional benefits. Along with that, however, we might want to adjust the LIBRESSL_VERSION_NUMBER check in the ngx_event_openssl.h file, so OPENSSL_VERSION_NUMBER is set to a better value for old LibreSSL versions - for example, to only set OPENSSL_VERSION_NUMBER to 0x101fL for LibreSSL 3.5.0 or above. This might allow to preserve limited compatibility with ancient LibreSSL versions without additional efforts (not tested though). -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello!
On Fri, Dec 15, 2023 at 03:46:19PM +0100, Dipl. Ing. Sergey Brester via
nginx-devel wrote:
> Enclosed few thoughts to the subject:
>
> - since it is very rare situation that one needs only a memcpy without
> to know whether previous alloc may fail
>(e. g. some of pointers were NULL), me too thinks that the caller
> should be responsible for the check.
>So I would not extend ngx_memcpy or ngx_cpymem in that way.
That's not about failed allocations, it's about ability to work
with empty strings which aren't explicitly set to something other
than { 0, NULL }. And you may refer to Vladimir's patch I've
referenced to find out what it means in terms of the "caller
should be responsible" approach even without trying to look into
places not explicitly reported by the UB sanitizer.
https://mailman.nginx.org/pipermail/nginx-devel/2023-October/PX7VH5A273NLUGSYC7DR2AZRU75CIQ3Q.html
https://mailman.nginx.org/pipermail/nginx-devel/2023-December/DCGUEGEFS6TSVIWNEWUEZO3FZMR6ESYZ.html
> - rewrite of `ngx_memcpy` define like here:
>```
>+ #define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) :
> memcpy(dst, src, n))
>```
>may introduce a regression or compat issues, e. g. fully functioning
> codes like that may become broken hereafter:
>```
>ngx_memcpy(dst, src, ++len); // because n would be incremented twice
> in the macro now
>```
>Sure, `ngx_cpymem` has also the same issue, but it had that already
> before the "fix"...
>Anyway, I'm always against of such macros (no matter with or without
> check it would be better as an inline function instead).
In general macro definitions in nginx are used everywhere for
efficiency reasons, and macro definitions usually aren't safe.
While some might prefer other approaches, writing code like
"ngx_memcpy(dst, src, ++len)" in nginx is just wrong, and
shouldn't be trusted to work, much like it won't work with
"ngx_cpymem(dst, src, ++len)".
I'm not exactly against using inline functions here, but the
particular argument is at most very weak.
> My conclusion:
>a fix of affected places invoking `ngx_memcpy` and `ngx_cpymem`, and
> possibly an assert in `ngx_memcpy`
>and `ngx_cpymem` would be fully enough, in my opinion.
Well, thank you for your opinion, appreciated. I don't think this
approach is going to work though, see my review of Vladimir's
patch.
Ideally, I would prefer this to be fixed in the C standard (and
GCC). But given this is not a likely option, and there is a
constant stream of reports "hey, UB sanitizer reports about
memcpy(dst, NULL, 0) in nginx" we might consider actually
silencing this by introducing appropriate checks at the interface
level.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 2 of 2] Win32: extended ngx_random range to 0x7fffffff
Hello! On Sat, Dec 09, 2023 at 08:42:11AM +, J Carter wrote: > On Sat, 09 Dec 2023 07:46:10 + > J Carter wrote: > > > # HG changeset patch > > # User J Carter > > # Date 1702101635 0 > > # Sat Dec 09 06:00:35 2023 + > > # Node ID 1a77698f82d2580aa8b8f62ce89b4dbb6d678c5d > > # Parent d9275e982a7188a1ea7855295ffa93362ea9830f > > Win32: extended ngx_random range to 0x7fff > > > > rand() is used on win32. RAND_MAX is implementation defined. win32's is > > 0x7fff. > > > > Existing uses of ngx_random rely upon 0x7fff range provided by > > posix implementations of random(). > > > > diff -r d9275e982a71 -r 1a77698f82d2 src/os/win32/ngx_win32_config.h > > --- a/src/os/win32/ngx_win32_config.h Sat Dec 09 05:09:07 2023 + > > +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 06:00:35 2023 + > > @@ -280,7 +280,9 @@ > > > > #define NGX_HAVE_GETADDRINFO 1 > > > > -#define ngx_random rand > > +#define ngx_random > > \ > > +((rand() << 16) | (rand() << 1) | (rand() >> 14)) > > + > > #define ngx_debug_init() > > > > > > ^ my mistake - copying error.. > > # HG changeset patch > # User J Carter > # Date 1702111094 0 > # Sat Dec 09 08:38:14 2023 + > # Node ID 10ef59a412a330872fc6d46de64f42e32e997b3a > # Parent d9275e982a7188a1ea7855295ffa93362ea9830f > Win32: extended ngx_random range to 0x7fff Nitpicking: Win32: extended ngx_random() range to 0x7fff. > > rand() is used on win32. RAND_MAX is implementation defined. win32's is > 0x7fff. > > Existing uses of ngx_random rely upon 0x7fff range provided by > posix implementations of random(). Thanks for catching this. As far as I can see, the only module which actually relies on the range is the random index module. Relying on the ngx_random() range generally looks wrong to me, and we might want to change the code to don't. OTOH, it's the only way to get a completely uniform distribution, and that's what the module tries to do. As such, it might be good enough to preserve it as is, at least till further changes to ngx_random(). Either way, wider range for ngx_random() should be beneficial in other places. > > diff -r d9275e982a71 -r 10ef59a412a3 src/os/win32/ngx_win32_config.h > --- a/src/os/win32/ngx_win32_config.h Sat Dec 09 05:09:07 2023 + > +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 08:38:14 2023 + > @@ -280,7 +280,9 @@ > > #define NGX_HAVE_GETADDRINFO 1 > > -#define ngx_random rand > +#define ngx_random() > \ Nitpicking: the "\" character should be at the 79th column (in some files at 78th). This ensures that a diff won't wrap on a standard 80-column terminal. > +((rand() << 16) | (rand() << 1) | (rand() >> 14)) > + > #define ngx_debug_init() Using "|" for random numbers looks utterly wrong to me, even if ORed values are guaranteed to not interfere. I would rather use "^", and avoid dependency on the particular value of RAND_MAX (other than POSIX-required minimum of 32767) by using something like 0x7fff & ((rand() << 16) ^ (rand() << 8) ^ rand()) with proper typecasts. Something like this should work: diff --git a/src/os/win32/ngx_win32_config.h b/src/os/win32/ngx_win32_config.h --- a/src/os/win32/ngx_win32_config.h +++ b/src/os/win32/ngx_win32_config.h @@ -280,7 +280,11 @@ typedef int sig_atomic_t #define NGX_HAVE_GETADDRINFO 1 -#define ngx_random rand +#define ngx_random() \ +((long) (0x7fff & ( ((uint32_t) rand() << 16) \ + ^ ((uint32_t) rand() << 8) \ + ^ ((uint32_t) rand()) ))) + #define ngx_debug_init() -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add ssl_client_tls_bind variable
Hello! On Fri, Dec 15, 2023 at 06:02:45PM +1100, Rob Casey wrote: > First time caller, long time listener. > > This patch introduces the variable $ssl_client_tls_bind which provides the > last Finished message returned by the OpenSSL SSL_get_peer_finished() > function. The value returned by this function may be used in TLS channel > binding operations as described in RFC 5929 > <https://datatracker.ietf.org/doc/html/rfc5929> (TLSv1.2) and RFC 9266 > <https://datatracker.ietf.org/doc/html/rfc9266> (TLSv1.3). The bytes > returned by this function are base64-encoded for ease-of-use as per > suggestion on Nginx forum thread > <https://forum.nginx.org/read.php?10,286777>. You might be interested in a previous attempt to introduce similar variables, here: https://mailman.nginx.org/pipermail/nginx-devel/2021-May/014082.html https://mailman.nginx.org/pipermail/nginx-devel/2021-June/014090.html -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello!
On Wed, Dec 13, 2023 at 11:09:28AM -0500, Ben Kallus wrote:
> Nginx executes numerous `memcpy`s from NULL during normal execution.
> `memcpy`ing to or from NULL is undefined behavior. Accordingly, some
> compilers (gcc -O2) make optimizations that assume `memcpy` arguments
> are not NULL. Nginx with UBSan crashes during startup due to this
> issue.
>
> Consider the following function:
> ```C
> #include
>
> int f(int i) {
> char a[] = {'a'};
> void *src = i ? a : NULL;
> char dst[1];
> memcpy(dst, src, 0);
> return src == NULL;
> }
> ```
> Here's what gcc13.2 -O2 -fno-builtin will do to it:
> ```asm
> f:
> sub rsp, 24
> xor eax, eax
> testedi, edi
> lea rsi, [rsp+14]
> lea rdi, [rsp+15]
> mov BYTE PTR [rsp+14], 97
> cmove rsi, rax
> xor edx, edx
> callmemcpy
> xor eax, eax
> add rsp, 24
> ret
> ```
> Note that `f` always returns 0, regardless of the value of `i`.
>
> Feel free to try for yourself at https://gcc.godbolt.org/z/zfvnMMsds
>
> The reasoning here is that since memcpy from NULL is UB, the optimizer
> is free to assume that `src` is non-null. You might consider this to
> be a problem with the compiler, or the C standard, and I might agree.
> Regardless, relying on UB is inherently un-portable, and requires
> maintenance to ensure that new compiler releases don't break existing
> assumptions about the behavior of undefined operations.
>
> The following patch adds a check to `ngx_memcpy` and `ngx_cpymem` that
> makes 0-length memcpy explicitly a noop. Since all memcpying from NULL
> in Nginx uses n==0, this should be sufficient to avoid UB.
>
> It would be more efficient to instead add a check to every call to
> ngx_memcpy and ngx_cpymem that might be used with src==NULL, but in
> the discussion of a previous patch that proposed such a change, a more
> straightforward and tidy solution was desired.
> It may also be worth considering adding checks for NULL memset,
> memmove, etc. I think this is not necessary unless it is demonstrated
> that Nginx actually executes such undefined calls.
>
> # HG changeset patch
> # User Ben Kallus
> # Date 1702406466 18000
> # Tue Dec 12 13:41:06 2023 -0500
> # Node ID d270203d4ecf77cc14a2652c727e236afc659f4a
> # Parent a6f79f044de58b594563ac03139cd5e2e6a81bdb
> Add NULL check to ngx_memcpy and ngx_cpymem to satisfy UBSan.
>
> diff -r a6f79f044de5 -r d270203d4ecf src/core/ngx_string.c
> --- a/src/core/ngx_string.c Wed Nov 29 10:58:21 2023 +0400
> +++ b/src/core/ngx_string.c Tue Dec 12 13:41:06 2023 -0500
> @@ -2098,6 +2098,10 @@
> ngx_debug_point();
> }
>
> +if (n == 0) {
> +return dst;
> +}
> +
> return memcpy(dst, src, n);
> }
>
> diff -r a6f79f044de5 -r d270203d4ecf src/core/ngx_string.h
> --- a/src/core/ngx_string.h Wed Nov 29 10:58:21 2023 +0400
> +++ b/src/core/ngx_string.h Tue Dec 12 13:41:06 2023 -0500
> @@ -103,8 +103,9 @@
> * gcc3 compiles memcpy(d, s, 4) to the inline "mov"es.
> * icc8 compile memcpy(d, s, 4) to the inline "mov"es or XMM moves.
> */
> -#define ngx_memcpy(dst, src, n) (void) memcpy(dst, src, n)
> -#define ngx_cpymem(dst, src, n) (((u_char *) memcpy(dst, src, n)) + (n))
> +#define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) : memcpy(dst, src,
> n))
> +#define ngx_cpymem(dst, src, n)
> \
> +((u_char *) ((n) == 0 ? (dst) : memcpy(dst, src, n)) + (n))
>
> #endif
>
> diff -r a6f79f044de5 -r d270203d4ecf src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c Wed Nov 29 10:58:21 2023 +0400
> +++ b/src/http/v2/ngx_http_v2.c Tue Dec 12 13:41:06 2023 -0500
> @@ -3998,9 +3998,7 @@
> n = size;
> }
>
> -if (n > 0) {
> -rb->buf->last = ngx_cpymem(rb->buf->last, pos, n);
> -}
> +rb->buf->last = ngx_cpymem(rb->buf->last, pos, n);
>
> ngx_log_debug1(NGX_LOG_DEBUG_HTTP, fc->log, 0,
> "http2 request body recv %uz", n);
For the record, I've already provided some feedback to Ben in the
ticket here:
https://trac.nginx.org/nginx/ticket/2570
And pointed to the existing thread here:
https://mailman.nginx.org/pipermail/nginx-devel/2023-October/PX7VH5A273NLUGSYC7DR2AZRU75CIQ3Q.html
https://mailman.nginx.org/pipermail/nginx-devel/2023-December/DCGUEGEFS6TSVIWNEWUEZO3FZMR6ESYZ.html
Hope this helps.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Added asm ISB as asm pause for ngx_cpu_pause() for aarch64
21198 > Difference at 95.0% confidence > 1044.47 +/- 435.826 > 0.800713% +/- 0.334115% > (Student's t, pooled s = 582.792) That's without any caching being used, that is, basically just a result of slightly different compilation, correct? This might be seen as a reference point of how slightly different compilation can affect performance. We've previously seen cases of 2-3% performance improvement observed as a result of a nop change, and these results seem to be in line. Tuning compilation to ensure there is no difference here might be the way to go. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: processing a request without body
Hello! On Wed, Dec 13, 2023 at 03:55:56PM +0800, Muhammad Nuzaihan wrote: > I need to process requests with only URI path (without body) for a module. > > It seems ngx_http_request_body_filter_pt is *not* executed whenever > there is a request without a body (it looked like it bypassed without > request body) and only ngx_http_output_body_filter_pt part of the > code is executed. > > For example i do a request curl curl like this: > > curl - -X POST http://localhost:8080/proxy/profile/alice/comment > > and i need to validate /proxy/profile/alice/comment in my module and > there is no http headers and no body. Only URI path. When reading an HTTP request, nginx reads the request line and request headers, and then starts request processing by passing it through as sequence of request processing phases - in each phase, corresponding handlers are called. The request body filters are only called if there is a request body, and some handler actually requested reading of the request body - so it's expected these are not called if there is no request body. If you want to handle request based on the request line and the request headers, consider using appropriate phase handler, see https://nginx.org/en/docs/dev/development_guide.html#http_phases and the source code for details. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Added asm ISB as asm pause for ngx_cpu_pause() for aarch64
81dd7f9b037b59b362/src/mongo/platform/pause.h#L61 > > MySQL: > https://github.com/mysql/mysql-server/blob/87307d4ddd88405117e3f1e51323836d57ab1f57/storage/innobase/include/ut0ut.h#L108 > > Jemalloc: > https://github.com/jemalloc/jemalloc/blob/e4817c8d89a2a413e835c4adeab5c5c4412f9235/configure.ac#L436 Thanks for the links. For the record, here are relevant commits / pull requests: https://github.com/wiredtiger/wiredtiger/pull/6080 https://github.com/mongodb/mongo/commit/6979525674af67405984c58585766dd4d0c3f2a8 https://bugs.mysql.com/bug.php?id=100664 https://github.com/mysql/mysql-server/commit/f2a4ed5b65a6c03ee1bea60b8c3bb4db64dbed10 https://github.com/jemalloc/jemalloc/pull/2205 https://github.com/jemalloc/jemalloc/commit/89fe8ee6bf7a23556350d883a310c0224a171879 At least MySQL bug seems to have some interesting details. > > Could you please clarify reasons for the "memory" clobber here? > > Putting in the memory clobber for ISB is redundant because ISB is a > barrier itself, but it's probably the GCC appropriate thing to do. I > also like it as a hint for someone not familiar with ISB. ISB will pause > the frontend (fetch-decode) to allow the CPU backend (execute-retire) to > finish whatever operations are in flight. It's possible that some of > those operations are writes to memory. Hence why we should tell the > compiler "this instruction may update memory". I don't think this interpretation is correct - memory is updated by other instructions, not ISB. And it's the compiler who emitted these instructions, so it perfectly knows that these will eventually update memory. Note that the ngx_cpu_pause() call does not need to be a barrier, neither a hardware barrier nor a compiler barrier. Instead, nginx relies on using volatile variables for locks, so these are always loaded from memory by the compiler on pre-lock checks (and will test the updated value if it's changed by other CPUs), and proper barrier semantics of ngx_atomic_cmp_set(). The "memory" clobber essentially tells compiler that it cannot optimize stores and loads across the call, and must reload anything from memory after the call. While this might be expected in other uses (for example, cpu_relax() in Linux is expected to be a compiler barrier), this is not something needed in ngx_cpu_pause(). [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Added asm ISB as asm pause for ngx_cpu_pause() for aarch64
Hello!
Thank you for the patch.
Some comments and questions below.
On Wed, Dec 06, 2023 at 10:06:57AM -0600, julio.sua...@foss.arm.com wrote:
> # HG changeset patch
> # User Julio Suarez
> # Date 1701877879 21600
> # Wed Dec 06 09:51:19 2023 -0600
> # Node ID 53d289b8676fc678ca90e02ece174300a3631f79
> # Parent f366007dd23a6ce8e8427c1b3042781b618a2ade
> Added asm ISB as asm pause for ngx_cpu_pause() for aarch64
Nitpicking:
Added ISB as ngx_cpu_pause() for aarch64.
>
> For aarch64 (A.K.A. Arm64), ngx_cpu_pause() evaluates to empty by the
> GCC preprocessor. This results in an empty for loop in the lock
> check code in ngx_rwlock.c/ngx_spinlock.c (a bug).
> Thus, on Arm CPUs, there is no wait between checks of a lock.
Could you please clarify what do you mean by "a bug"? An empty
ngx_cpu_pause() is certainly not a bug, it's just a lack of a more
optimal solution for the particular architecture.
> When a delay like the PAUSE that is there for x86 is added,
> there is a 2-5% increase in the number of requests/sec Arm CPUs
> can achieve under high load.
Could you please provide some details on how did you get these
numbers?
> Currently, options for a spin lock delay element
> are YIELD and ISB. YIELD is assembled into a NOP for most Arm CPUs.
> YIELD is for Arm implementations with SMT.
> Most Arm implementations are single core - single thread (non-SMT).
> Thus, this commit uses ISB (Instruction Barrier Sync) since it
> will provide a slightly longer delay than a NOP.
>
> Other projects that implement spin locks have used ISB as
> the delay element which has yielded performance gains as well.
Looking through various open source projects, I'm seeing the
following uses on arm64 CPUs:
FreeBSD, sys/arm64/include/cpu.h:
#define cpu_spinwait() __asm __volatile("yield" ::: "memory")
FreeBSD, lib/libthr/arch/aarch64/include/pthread_md.h:
#define CPU_SPINWAIT
Linux, arch/arm64/include/asm/vdso/processor.h:
static inline void cpu_relax(void)
{
asm volatile("yield" ::: "memory");
}
The only popular project I was able to find which uses ISB is
Rust:
https://github.com/rust-lang/rust/commit/c064b6560b7ce0adeb9bbf5d7dcf12b1acb0c807
From the commit log it looks like it mostly focuses on the delay
introduced by the instruction, ignoring other effects. In
particular, YIELD is expected to be more friendly for various
emulation environments, see Linux commit here:
https://github.com/torvalds/linux/commit/1baa82f48030f38d1895301f1ec93acbcb3d15db
Overall, the YIELD instruction seems to be better suited and
specifically designed for the task in question, as per the
documentation
(https://developer.arm.com/documentation/ddi0596/2021-12/Base-Instructions/YIELD--YIELD-):
: YIELD is a hint instruction. Software with a multithreading
: capability can use a YIELD instruction to indicate to the PE that
: it is performing a task, for example a spin-lock, that could be
: swapped out to improve overall system performance. The PE can use
: this hint to suspend and resume multiple software threads if it
: supports the capability.
Please also note that ngx_cpu_pause() is only used on
multiprocessor systems: the code checks if (ngx_ncpu > 1) before
using it.
>
> Last, ISB is not an architecturally defined solution
> for short spin lock delays.
> A candidate for a short delay solution is the WFET
> instruction (Wait For Event with Timeout).
> However, there aren't any Arm implementations in the market that
> support this instruction yet. When that occurs,
> WFET can be tested as a replacement for ISB. Until then,
> ISB will do.
>
> diff -r f366007dd23a -r 53d289b8676f src/os/unix/ngx_atomic.h
> --- a/src/os/unix/ngx_atomic.hTue Nov 14 15:26:02 2023 +0400
> +++ b/src/os/unix/ngx_atomic.hWed Dec 06 09:51:19 2023 -0600
> @@ -66,6 +66,8 @@
>
> #if ( __i386__ || __i386 || __amd64__ || __amd64 )
> #define ngx_cpu_pause() __asm__ ("pause")
> +#elif ( __aarch64__ )
> +#define ngx_cpu_pause() __asm__ ("isb" ::: "memory")
Could you please clarify reasons for the "memory" clobber here?
> #else
> #define ngx_cpu_pause()
> #endif
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Error: nginx: [emerg] unknown directive "stream" in /etc/nginx/nginx.conf:11
Hello! On Thu, Dec 07, 2023 at 05:34:23PM +0300, Sergey A. Osokin wrote: > On Thu, Dec 07, 2023 at 09:17:37AM +, Gandla, Kesavardhana via nginx > wrote: > > Dear NGINX community, > > > > I am Kesavardhana Gandla from Medtronic R, Bangalore. I am trying to > > evaluate the nginx on YOCTO Linux based embedded product. > > My Linux version: 5.15.71 aarch64 GNU/Linux. > > While using the stream option on target the below error is coming. > > > > Error: nginx: [emerg] unknown directive "stream" in /etc/nginx/nginx.conf:11 > > Please avoid cross-posting to multiple mailing lists. Since this > question is related to nginx development, it can be addressed to > nginx-devel mailing list. Certainly this is not related to nginx development. For user-level questions, please use the nginx@ mailing list. Alternatively, reading the docs might be a good idea. Thanks for understanding. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] Core: avoid calling memcpy() in edge cases
; diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
> --- a/src/http/ngx_http_file_cache.c
> +++ b/src/http/ngx_http_file_cache.c
> @@ -1270,7 +1270,9 @@ ngx_http_file_cache_set_header(ngx_http_
>
> if (c->etag.len <= NGX_HTTP_CACHE_ETAG_LEN) {
> h->etag_len = (u_char) c->etag.len;
> -ngx_memcpy(h->etag, c->etag.data, c->etag.len);
> +if (c->etag.len) {
> +ngx_memcpy(h->etag, c->etag.data, c->etag.len);
> +}
> }
>
> if (c->vary.len) {
> diff --git a/src/http/ngx_http_variables.c b/src/http/ngx_http_variables.c
> --- a/src/http/ngx_http_variables.c
> +++ b/src/http/ngx_http_variables.c
> @@ -2157,6 +2157,9 @@ ngx_http_variable_request_body(ngx_http_
>
> for ( /* void */ ; cl; cl = cl->next) {
> buf = cl->buf;
> +if (buf->last == buf->pos) {
> +continue;
> +}
> p = ngx_cpymem(p, buf->pos, buf->last - buf->pos);
> }
>
> diff --git a/src/mail/ngx_mail_auth_http_module.c
> b/src/mail/ngx_mail_auth_http_module.c
> --- a/src/mail/ngx_mail_auth_http_module.c
> +++ b/src/mail/ngx_mail_auth_http_module.c
> @@ -1314,11 +1314,15 @@ ngx_mail_auth_http_create_request(ngx_ma
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-User: ", sizeof("Auth-User: ") - 1);
> -b->last = ngx_copy(b->last, login.data, login.len);
> +if (login.len) {
> +b->last = ngx_copy(b->last, login.data, login.len);
> +}
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-Pass: ", sizeof("Auth-Pass: ") - 1);
> -b->last = ngx_copy(b->last, passwd.data, passwd.len);
> +if (passwd.len) {
> +b->last = ngx_copy(b->last, passwd.data, passwd.len);
> +}
> *b->last++ = CR; *b->last++ = LF;
>
> if (s->auth_method != NGX_MAIL_AUTH_PLAIN && s->salt.len) {
> @@ -1375,7 +1379,9 @@ ngx_mail_auth_http_create_request(ngx_ma
>
> b->last = ngx_cpymem(b->last, "Auth-SMTP-Helo: ",
> sizeof("Auth-SMTP-Helo: ") - 1);
> -b->last = ngx_copy(b->last, s->smtp_helo.data, s->smtp_helo.len);
> +if (s->smtp_helo.len) {
> +b->last = ngx_copy(b->last, s->smtp_helo.data, s->smtp_helo.len);
> +}
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-SMTP-From: ",
If at all, these probably should check for login.len, passwd.len,
and s->smtp_helo.len, similarly to other cases nearby, and avoid
sending the headers altogether.
> diff --git a/src/stream/ngx_stream_script.c b/src/stream/ngx_stream_script.c
> --- a/src/stream/ngx_stream_script.c
> +++ b/src/stream/ngx_stream_script.c
> @@ -842,7 +842,9 @@ ngx_stream_script_copy_var_code(ngx_stre
>
> if (value && !value->not_found) {
> p = e->pos;
> -e->pos = ngx_copy(p, value->data, value->len);
> +if (value->len) {
> +e->pos = ngx_copy(p, value->data, value->len);
> +}
>
> ngx_log_debug2(NGX_LOG_DEBUG_STREAM,
> e->session->connection->log, 0,
Obviously enough, there should be a corresponding change in
ngx_http_script_copy_var_code().
Overall, similarly to the other patch in the series, I'm highly
sceptical about doing such scattered changes based on the UB
sanitizer reports from some test runs. Rather, we should use UB
sanitizer reports to identify problematic patterns, and fix these
patterns all other the code (if at all).
Further, in this particular case I tend to think that the problem
is not with nginx code, but rather with the memcpy() interface UB
sanitizer tries to enforce. It should be completely safe to call
memcpy(p, NULL, 0), and if it doesn't, we might consider adding
appropriate guards at interface level, such as in ngx_memcpy() /
ngx_cpymem() wrappers, and not in each call. Trying to check
length everywhere is just ugly and unreadable.
Also, while recent versions of gcc are known to miscompile some
code which uses memcpy(p, NULL, 0) (see [1], with "-O2" or with
"-O1 -ftree-vrp" optimization flags in my tests), I don't think
this affects nginx code. If it does, we might also consider
force-switching off relevant optimizations (if enabled, as we use
"-O1" by default).
[1]
https://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer
Hello!
On Wed, Nov 29, 2023 at 11:24:03AM +0300, Vladimir Homutov via nginx-devel
wrote:
> On Tue, Nov 28, 2023 at 05:58:23AM +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Fri, Nov 10, 2023 at 12:11:54PM +0300, Vladimir Homutov via nginx-devel
> > wrote:
> >
> > > If URI is not fully parsed yet, some pointers are not set.
> > > As a result, the calculation of "new + (ptr - old)" expression
> > > may overflow. In such a case, just avoid calculating it, as value
> > > will be set correctly later by the parser in any case.
> > >
> > > The issue was found by GCC undefined behaviour sanitizer.
> > >
> > >
> > > src/http/ngx_http_request.c | 34 ++
> > > 1 files changed, 26 insertions(+), 8 deletions(-)
> > >
> > >
> >
> > > # HG changeset patch
> > > # User Vladimir Khomutov
> > > # Date 1699604478 -10800
> > > # Fri Nov 10 11:21:18 2023 +0300
> > > # Node ID 505e927eb7a75f0fdce4caddb4ab9d9c71c9b9c8
> > > # Parent dadd13fdcf5228c8e8380e120d4621002e3b0919
> > > HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer.
> > >
> > > If URI is not fully parsed yet, some pointers are not set.
> > > As a result, the calculation of "new + (ptr - old)" expression
> > > may overflow. In such a case, just avoid calculating it, as value
> > > will be set correctly later by the parser in any case.
> > >
> > > The issue was found by GCC undefined behaviour sanitizer.
> >
> > I would rather refrain from saying this is an issue, it is not
> > (unless a compiler actually starts to do silly things as long as
> > it sees something formally defined as "undefined behavior" in C
> > standard, and this would be indeed an issue - in the compiler).
> > As already noted in the initial review, the code as written is
> > completely safe in practice. For such mostly style commits we
> > usually write something like "Prodded by...".
>
> totally agreed
>
> >
> > Also note that the issue is not an overflow, but rather
> > subtraction of pointers which do not belong to the same array
> > object (C11, 6.5.6 Additive operators, p.9):
> >
> > : When two pointers are subtracted, both shall point to elements
> > : of the same array object, or one past the last element of the
> > : array object
> >
> > The undefined behaviour is present as long as "ptr" and "old" are
> > not in the same buffer (i.e., array object), which is the case
> > when "ptr" is not set. And another one follows when trying to add
> > the (already undefined) subtraction result to "new" (since the
> > result is not going to belong to the same array object):
> >
> > : If both the pointer operand and the result point to elements of
> > : the same array object, or one past the last element of the array
> > : object, the evaluation shall not produce an overflow; otherwise,
> > : the behavior is undefined.
> >
> > Overflow here might be an indicator that certainly there is an
> > undefined behaviour, but it's just an indicator.
> >
> > You may want to rewrite commit log accordingly.
>
> The commit log was updated.
>
> > > diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> > > --- a/src/http/ngx_http_request.c
> > > +++ b/src/http/ngx_http_request.c
> > > @@ -1718,14 +1718,23 @@ ngx_http_alloc_large_header_buffer(ngx_h
> [...]
> > > if (r->host_start) {
> >
> > See review of the second patch about r->port_start / r->port_end.
> > I would rather change it similarly for now.
>
> I would prefer to remove both, so this patch has nothing about it.
>
> updated patch:
>
>
> # HG changeset patch
> # User Vladimir Khomutov
> # Date 1701245585 -10800
> # Wed Nov 29 11:13:05 2023 +0300
> # Node ID 7c8ecb3fee20dfbb9a627441377dd09509988e2a
> # Parent dacad3a9c7b8435a4c67ad2b67f261e7b4e36d8e
> HTTP: uniform checks in ngx_http_alloc_large_header_buffer().
>
> If URI is not fully parsed yet, some pointers are not set. As a result,
> the calculation of "new + (ptr - old)" expression is flawed.
>
> According to C11, 6.5.6 Additive operators, p.9:
Seems to be an extra space in "to C11".
>
> : When two pointers are subtracted, both shall point to elements
> : of the same array object, or one past the last element of the
> : array object
>
> Since &qu
Re: [PATCH 2 of 2] HTTP: removed unused r->port_start
Hello!
On Wed, Nov 29, 2023 at 11:20:33AM +0300, Vladimir Homutov via nginx-devel
wrote:
> On Tue, Nov 28, 2023 at 05:57:39AM +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Fri, Nov 10, 2023 at 12:11:55PM +0300, Vladimir Homutov via nginx-devel
> > wrote:
> >
> > >
> > > It is no longer used since the refactoring in 8e5bf1bc87e2 (2008).
> >
> > Neither r->port_start nor r->port_end were ever used.
> >
> > The r->port_end is set by the parser, though it was never used by
> > the following code (and was never usable, since not copied by the
> > ngx_http_alloc_large_header_buffer() without r->port_start set).
> >
> > The 8e5bf1bc87e2 commit is completely unrelated, it is about
> > refactoring of the ngx_parse_inet_url() function, which had a
> > local variable named "port_start".
>
> exactly, thanks for noticing.
>
> >
> > >
> > > diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> > > --- a/src/http/ngx_http_request.c
> > > +++ b/src/http/ngx_http_request.c
> > > @@ -1744,8 +1744,7 @@ ngx_http_alloc_large_header_buffer(ngx_h
> > > }
> > > }
> > >
> > > -if (r->port_start) {
> > > -r->port_start = new + (r->port_start - old);
> > > +if (r->port_end) {
> > > r->port_end = new + (r->port_end - old);
> > > }
> > >
> > > diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h
> > > --- a/src/http/ngx_http_request.h
> > > +++ b/src/http/ngx_http_request.h
> > > @@ -597,7 +597,6 @@ struct ngx_http_request_s {
> > > u_char *schema_end;
> > > u_char *host_start;
> > > u_char *host_end;
> > > -u_char *port_start;
> > > u_char *port_end;
> > >
> > > unsigned http_minor:16;
> >
> > I don't think it's a good change. Rather, we should either remove
> > both, or (eventually) fix these and provide some valid usage of
> > the port as parsed either from the request line or from the Host
> > header, similarly to the $host variable.
> >
>
> I think that we should remove both, as unused code still needs to be
> maintained without any advantage, as this example shows.
> Restoring it will be trivial, if ever required.
>
>
>
> # HG changeset patch
> # User Vladimir Khomutov
> # Date 1701165434 -10800
> # Tue Nov 28 12:57:14 2023 +0300
> # Node ID dacad3a9c7b8435a4c67ad2b67f261e7b4e36d8e
> # Parent f366007dd23a6ce8e8427c1b3042781b618a2ade
> HTTP: removed unused r->port_start and r->port_end.
>
> Neither r->port_start nor r->port_end were ever used.
>
> The r->port_end is set by the parser, though it was never used by
> the following code (and was never usable, since not copied by the
> ngx_http_alloc_large_header_buffer() without r->port_start set).
>
> diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c
> --- a/src/http/ngx_http_parse.c
> +++ b/src/http/ngx_http_parse.c
> @@ -451,19 +451,16 @@ ngx_http_parse_request_line(ngx_http_req
>
> switch (ch) {
> case '/':
> -r->port_end = p;
> r->uri_start = p;
> state = sw_after_slash_in_uri;
> break;
> case '?':
> -r->port_end = p;
> r->uri_start = p;
> r->args_start = p + 1;
> r->empty_path_in_uri = 1;
> state = sw_uri;
> break;
> case ' ':
> -r->port_end = p;
> /*
> * use single "/" from request line to preserve pointers,
> * if request line will be copied to large client buffer
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -1735,11 +1735,6 @@ ngx_http_alloc_large_header_buffer(ngx_h
> }
> }
>
> -if (r->port_start) {
> -r->port_start = new + (r->port_start - old);
> -r->port_end = new + (r->port_end - old);
> -}
> -
> if (r->uri_ext) {
> r->uri_ext = new + (r->uri_ext - old);
> }
> diff --git a/src/http/ngx_http_r
Re: [PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer
Hello!
On Fri, Nov 10, 2023 at 12:11:54PM +0300, Vladimir Homutov via nginx-devel
wrote:
> If URI is not fully parsed yet, some pointers are not set.
> As a result, the calculation of "new + (ptr - old)" expression
> may overflow. In such a case, just avoid calculating it, as value
> will be set correctly later by the parser in any case.
>
> The issue was found by GCC undefined behaviour sanitizer.
>
>
> src/http/ngx_http_request.c | 34 ++
> 1 files changed, 26 insertions(+), 8 deletions(-)
>
>
> # HG changeset patch
> # User Vladimir Khomutov
> # Date 1699604478 -10800
> # Fri Nov 10 11:21:18 2023 +0300
> # Node ID 505e927eb7a75f0fdce4caddb4ab9d9c71c9b9c8
> # Parent dadd13fdcf5228c8e8380e120d4621002e3b0919
> HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer.
>
> If URI is not fully parsed yet, some pointers are not set.
> As a result, the calculation of "new + (ptr - old)" expression
> may overflow. In such a case, just avoid calculating it, as value
> will be set correctly later by the parser in any case.
>
> The issue was found by GCC undefined behaviour sanitizer.
I would rather refrain from saying this is an issue, it is not
(unless a compiler actually starts to do silly things as long as
it sees something formally defined as "undefined behavior" in C
standard, and this would be indeed an issue - in the compiler).
As already noted in the initial review, the code as written is
completely safe in practice. For such mostly style commits we
usually write something like "Prodded by...".
Also note that the issue is not an overflow, but rather
subtraction of pointers which do not belong to the same array
object (C11, 6.5.6 Additive operators, p.9):
: When two pointers are subtracted, both shall point to elements
: of the same array object, or one past the last element of the
: array object
The undefined behaviour is present as long as "ptr" and "old" are
not in the same buffer (i.e., array object), which is the case
when "ptr" is not set. And another one follows when trying to add
the (already undefined) subtraction result to "new" (since the
result is not going to belong to the same array object):
: If both the pointer operand and the result point to elements of
: the same array object, or one past the last element of the array
: object, the evaluation shall not produce an overflow; otherwise,
: the behavior is undefined.
Overflow here might be an indicator that certainly there is an
undefined behaviour, but it's just an indicator.
You may want to rewrite commit log accordingly.
>
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -1718,14 +1718,23 @@ ngx_http_alloc_large_header_buffer(ngx_h
> r->request_end = new + (r->request_end - old);
> }
>
> -r->method_end = new + (r->method_end - old);
> -
> -r->uri_start = new + (r->uri_start - old);
> -r->uri_end = new + (r->uri_end - old);
> +if (r->method_end) {
> +r->method_end = new + (r->method_end - old);
> +}
> +
> +if (r->uri_start) {
> +r->uri_start = new + (r->uri_start - old);
> +}
> +
> +if (r->uri_end) {
> +r->uri_end = new + (r->uri_end - old);
> +}
>
> if (r->schema_start) {
> r->schema_start = new + (r->schema_start - old);
> -r->schema_end = new + (r->schema_end - old);
> +if (r->schema_end) {
> +r->schema_end = new + (r->schema_end - old);
> +}
> }
>
> if (r->host_start) {
See review of the second patch about r->port_start / r->port_end.
I would rather change it similarly for now.
> @@ -1754,9 +1763,18 @@ ngx_http_alloc_large_header_buffer(ngx_h
>
> } else {
> r->header_name_start = new;
> -r->header_name_end = new + (r->header_name_end - old);
> -r->header_start = new + (r->header_start - old);
> -r->header_end = new + (r->header_end - old);
> +
> +if (r->header_name_end) {
> +r->header_name_end = new + (r->header_name_end - old);
> +}
> +
> +if (r->header_start) {
> +r->header_start = new + (r->header_start - old);
> +}
> +
> +if (r->header_end) {
> +r->header_end = new + (r->header_end - old);
> +}
> }
>
> r->header_in = b;
Otherwise looks good.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 2 of 2] HTTP: removed unused r->port_start
Hello!
On Fri, Nov 10, 2023 at 12:11:55PM +0300, Vladimir Homutov via nginx-devel
wrote:
> It is no longer used since the refactoring in 8e5bf1bc87e2 (2008).
>
>
> src/http/ngx_http_request.c | 3 +--
> src/http/ngx_http_request.h | 1 -
> 2 files changed, 1 insertions(+), 3 deletions(-)
>
>
> # HG changeset patch
> # User Vladimir Khomutov
> # Date 1699603821 -10800
> # Fri Nov 10 11:10:21 2023 +0300
> # Node ID 6f957e137407d8f3f7e34f413c92103004b44594
> # Parent 505e927eb7a75f0fdce4caddb4ab9d9c71c9b9c8
> HTTP: removed unused r->port_start.
>
> It is no longer used since the refactoring in 8e5bf1bc87e2 (2008).
Neither r->port_start nor r->port_end were ever used.
The r->port_end is set by the parser, though it was never used by
the following code (and was never usable, since not copied by the
ngx_http_alloc_large_header_buffer() without r->port_start set).
The 8e5bf1bc87e2 commit is completely unrelated, it is about
refactoring of the ngx_parse_inet_url() function, which had a
local variable named "port_start".
>
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -1744,8 +1744,7 @@ ngx_http_alloc_large_header_buffer(ngx_h
> }
> }
>
> -if (r->port_start) {
> -r->port_start = new + (r->port_start - old);
> +if (r->port_end) {
> r->port_end = new + (r->port_end - old);
> }
>
> diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h
> --- a/src/http/ngx_http_request.h
> +++ b/src/http/ngx_http_request.h
> @@ -597,7 +597,6 @@ struct ngx_http_request_s {
> u_char *schema_end;
> u_char *host_start;
> u_char *host_end;
> -u_char *port_start;
> u_char *port_end;
>
> unsigned http_minor:16;
I don't think it's a good change. Rather, we should either remove
both, or (eventually) fix these and provide some valid usage of
the port as parsed either from the request line or from the Host
header, similarly to the $host variable.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
[PATCH 1 of 4] Fixed request termination with AIO and subrequests (ticket #2555)
# HG changeset patch
# User Maxim Dounin
# Date 1701049682 -10800
# Mon Nov 27 04:48:02 2023 +0300
# Node ID a5e39e9d1f4c84dcbe6a2f9e079372a3d63aef0b
# Parent f366007dd23a6ce8e8427c1b3042781b618a2ade
Fixed request termination with AIO and subrequests (ticket #2555).
When a request was terminated due to an error via ngx_http_terminate_request()
while an AIO operation was running in a subrequest, various issues were
observed. This happened because ngx_http_request_finalizer() was only set
in the subrequest where ngx_http_terminate_request() was called, but not
in the subrequest where the AIO operation was running. After completion
of the AIO operation resumed normal processing of the subrequest, leading
to issues.
In particular, in case of the upstream module, termination of the request
called upstream cleanup, which closed the upstream connection. Attempts to
further work with the upstream connection after AIO operation completion
resulted in segfaults in ngx_ssl_recv(), "readv() failed (9: Bad file
descriptor) while reading upstream" errors, or socket leaks.
In ticket #2555, issues were observed with the following configuration
with cache background update (with thread writing instrumented to
introduce a delay, when a client closes the connection during an update):
location = /background-and-aio-write {
proxy_pass ...
proxy_cache one;
proxy_cache_valid 200 1s;
proxy_cache_background_update on;
proxy_cache_use_stale updating;
aio threads;
aio_write on;
limit_rate 1000;
}
Similarly, the same issue can be seen with SSI, and can be caused by
errors in subrequests, such as in the following configuration
(were "/proxy" uses AIO, and "/sleep" returns 444 after some delay,
causing request termination):
location = /ssi-active-boom {
ssi on;
ssi_types *;
return 200 '
';
limit_rate 1000;
}
Or the same with both AIO operation and the error in non-active subrequests
(which needs slightly different handling, see below):
location = /ssi-non-active-boom {
ssi on;
ssi_types *;
return 200 '
';
limit_rate 1000;
}
Similarly, issues can be observed with just static files. However,
with static files potential impact is limited due to timeout safeguards
in ngx_http_writer(), and the fact that c->error is set during request
termination.
In a simple configuration with an AIO operation in the active subrequest,
such as in the following configuration, the connection is closed right
after completion of the AIO operation anyway, since ngx_http_writer()
tries to write to the connection and fails due to c->error set:
location = /ssi-active-static-boom {
ssi on;
ssi_types *;
return 200 '
';
limit_rate 1000;
}
In the following configuration, with an AIO operation in a non-active
subrequest, the connection is closed only after send_timeout expires:
location = /ssi-non-active-static-boom {
ssi on;
ssi_types *;
return 200 '
';
limit_rate 1000;
}
Fix is to introduce r->main->terminated flag, which is to be checked
by AIO event handlers when the r->main->blocked counter is decremented.
When the flag is set, handlers are expected to wake up the connection
instead of the subrequest (which might be already cleaned up).
Additionally, now ngx_http_request_finalizer() is always set in the
active subrequest, so waking up the connection properly finalizes the
request even if termination happened in a non-active subrequest.
diff --git a/src/http/ngx_http_copy_filter_module.c
b/src/http/ngx_http_copy_filter_module.c
--- a/src/http/ngx_http_copy_filter_module.c
+++ b/src/http/ngx_http_copy_filter_module.c
@@ -195,9 +195,18 @@ ngx_http_copy_aio_event_handler(ngx_even
r->main->blocked--;
r->aio = 0;
-r->write_event_handler(r);
+if (r->main->terminated) {
+/*
+ * trigger connection event handler if the request was
+ * terminated
+ */
-ngx_http_run_posted_requests(c);
+c->write->handler(c->write);
+
+} else {
+r->write_event_handler(r);
+ngx_http_run_posted_requests(c);
+}
}
#endif
@@ -305,11 +314,11 @@ ngx_http_copy_thread_event_handler(ngx_e
#endif
-if (r->done) {
+if (r->done || r->main->terminated) {
/*
* trigger connection event handler if the subrequest was
- * already finalized; this can happen if the handler is used
- * for sendfile() in threads
+ * already finalized (this c
[PATCH 4 of 4] AIO operations now add timers (ticket #2162)
# HG changeset patch
# User Maxim Dounin
# Date 1701050170 -10800
# Mon Nov 27 04:56:10 2023 +0300
# Node ID 00c3e7333145ddb5ea0eeaaa66b3d9c26973c9c2
# Parent 61d08e4cf97cc073200ec32fc6ada9a2d48ffe51
AIO operations now add timers (ticket #2162).
Each AIO (thread IO) operation being run is now accompanied with 1-minute
timer. This timer prevents unexpected shutdown of the worker process while
an AIO operation is running, and logs an alert if the operation is running
for too long.
This fixes "open socket left" alerts during worker processes shutdown
due to pending AIO (or thread IO) operations while corresponding requests
have no timers. In particular, such errors were observed while reading
cache headers (ticket #2162), and with worker_shutdown_timeout.
diff --git a/src/http/ngx_http_copy_filter_module.c
b/src/http/ngx_http_copy_filter_module.c
--- a/src/http/ngx_http_copy_filter_module.c
+++ b/src/http/ngx_http_copy_filter_module.c
@@ -170,6 +170,8 @@ ngx_http_copy_aio_handler(ngx_output_cha
file->aio->data = r;
file->aio->handler = ngx_http_copy_aio_event_handler;
+ngx_add_timer(>aio->event, 6);
+
r->main->blocked++;
r->aio = 1;
ctx->aio = 1;
@@ -192,6 +194,17 @@ ngx_http_copy_aio_event_handler(ngx_even
ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
"http aio: \"%V?%V\"", >uri, >args);
+if (ev->timedout) {
+ngx_log_error(NGX_LOG_ALERT, c->log, 0,
+ "aio operation took too long");
+ev->timedout = 0;
+return;
+}
+
+if (ev->timer_set) {
+ngx_del_timer(ev);
+}
+
r->main->blocked--;
r->aio = 0;
@@ -273,6 +286,8 @@ ngx_http_copy_thread_handler(ngx_thread_
return NGX_ERROR;
}
+ngx_add_timer(>event, 6);
+
r->main->blocked++;
r->aio = 1;
@@ -297,6 +312,17 @@ ngx_http_copy_thread_event_handler(ngx_e
ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
"http thread: \"%V?%V\"", >uri, >args);
+if (ev->timedout) {
+ngx_log_error(NGX_LOG_ALERT, c->log, 0,
+ "thread operation took too long");
+ev->timedout = 0;
+return;
+}
+
+if (ev->timer_set) {
+ngx_del_timer(ev);
+}
+
r->main->blocked--;
r->aio = 0;
diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
--- a/src/http/ngx_http_file_cache.c
+++ b/src/http/ngx_http_file_cache.c
@@ -705,6 +705,8 @@ ngx_http_file_cache_aio_read(ngx_http_re
c->file.aio->data = r;
c->file.aio->handler = ngx_http_cache_aio_event_handler;
+ngx_add_timer(>file.aio->event, 6);
+
r->main->blocked++;
r->aio = 1;
@@ -752,6 +754,17 @@ ngx_http_cache_aio_event_handler(ngx_eve
ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
"http file cache aio: \"%V?%V\"", >uri, >args);
+if (ev->timedout) {
+ngx_log_error(NGX_LOG_ALERT, c->log, 0,
+ "aio operation took too long");
+ev->timedout = 0;
+return;
+}
+
+if (ev->timer_set) {
+ngx_del_timer(ev);
+}
+
r->main->blocked--;
r->aio = 0;
@@ -810,6 +823,8 @@ ngx_http_cache_thread_handler(ngx_thread
return NGX_ERROR;
}
+ngx_add_timer(>event, 6);
+
r->main->blocked++;
r->aio = 1;
@@ -831,6 +846,17 @@ ngx_http_cache_thread_event_handler(ngx_
ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
"http file cache thread: \"%V?%V\"", >uri, >args);
+if (ev->timedout) {
+ngx_log_error(NGX_LOG_ALERT, c->log, 0,
+ "thread operation took too long");
+ev->timedout = 0;
+return;
+}
+
+if (ev->timer_set) {
+ngx_del_timer(ev);
+}
+
r->main->blocked--;
r->aio = 0;
diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -3949,6 +3949,8 @@ ngx_http_upstream_thread_handler(ngx_thr
r->aio = 1;
p->aio = 1;
+ngx_add_timer(>event, 6);
+
return NGX_OK;
}
@@ -3967,6 +3969,17 @@ ngx_http_upstream_thread_event_handler(n
ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
"http upstream thread: \"%V?%V\"", >uri, >args);
+if (ev->timedout) {
+ngx_log_error(NGX_LOG_ALERT, c->log, 0,
+ "thread operation took too long");
+ev->timedout = 0;
+return;
+}
+
+if (ev->timer_set) {
+ngx_del_t
[PATCH 2 of 4] Upstream: fixed usage of closed sockets with filter finalization
# HG changeset patch
# User Maxim Dounin
# Date 1701049758 -10800
# Mon Nov 27 04:49:18 2023 +0300
# Node ID faf0b9defc76b8683af466f8a950c2c241382970
# Parent a5e39e9d1f4c84dcbe6a2f9e079372a3d63aef0b
Upstream: fixed usage of closed sockets with filter finalization.
When filter finalization is triggered when working with an upstream server,
and error_page redirects request processing to some simple handler,
ngx_http_request_finalize() triggers request termination when the response
is sent. In particular, via the upstream cleanup handler, nginx will close
the upstream connection and the corresponding socket.
Still, this can happen to be with ngx_event_pipe() on stack. While
the code will set p->downstream_error due to NGX_ERROR returned from the
output filter chain by filter finalization, otherwise the error will be
ignored till control returns to ngx_http_upstream_process_request().
And event pipe might try reading from the (already closed) socket, resulting
in "readv() failed (9: Bad file descriptor) while reading upstream" errors
(or even segfaults with SSL).
Such errors were seen with the following configuration:
location /t2 {
proxy_pass http://127.0.0.1:8080/big;
image_filter_buffer 10m;
image_filter resize 150 100;
error_page 415 = /empty;
}
location /empty {
return 204;
}
location /big {
# big enough static file
}
Fix is to set p->upstream_error in ngx_http_upstream_finalize_request(),
so the existing checks in ngx_event_pipe_read_upstream() will prevent
further reading from the closed upstream connection.
Similarly, p->upstream_error is now checked when handling events at
ngx_event_pipe() exit, as checking p->upstream->fd is not enough if
keepalive upstream connections are being used and the connection was
saved to cache during request termination.
diff --git a/src/event/ngx_event_pipe.c b/src/event/ngx_event_pipe.c
--- a/src/event/ngx_event_pipe.c
+++ b/src/event/ngx_event_pipe.c
@@ -57,7 +57,9 @@ ngx_event_pipe(ngx_event_pipe_t *p, ngx_
do_write = 1;
}
-if (p->upstream->fd != (ngx_socket_t) -1) {
+if (p->upstream->fd != (ngx_socket_t) -1
+&& !p->upstream_error)
+{
rev = p->upstream->read;
flags = (rev->eof || rev->error) ? NGX_CLOSE_EVENT : 0;
diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -4561,6 +4561,10 @@ ngx_http_upstream_finalize_request(ngx_h
u->peer.connection = NULL;
+if (u->pipe) {
+u->pipe->upstream_error = 1;
+}
+
if (u->pipe && u->pipe->temp_file) {
ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
"http upstream temp fd: %d",
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
[PATCH 3 of 4] Silenced complaints about socket leaks on forced termination
# HG changeset patch
# User Maxim Dounin
# Date 1701049787 -10800
# Mon Nov 27 04:49:47 2023 +0300
# Node ID 61d08e4cf97cc073200ec32fc6ada9a2d48ffe51
# Parent faf0b9defc76b8683af466f8a950c2c241382970
Silenced complaints about socket leaks on forced termination.
When graceful shutdown was requested, and then nginx was forced to
do fast shutdown, it used to (incorrectly) complain about open sockets
left in connections which weren't yet closed when fast shutdown
was requested.
Fix is to avoid complaining about open sockets when fast shutdown was
requested after graceful one. Abnormal termination, if requested with
the WINCH signal, can still happen though.
diff --git a/src/os/unix/ngx_process_cycle.c b/src/os/unix/ngx_process_cycle.c
--- a/src/os/unix/ngx_process_cycle.c
+++ b/src/os/unix/ngx_process_cycle.c
@@ -948,7 +948,7 @@ ngx_worker_process_exit(ngx_cycle_t *cyc
}
}
-if (ngx_exiting) {
+if (ngx_exiting && !ngx_terminate) {
c = cycle->connections;
for (i = 0; i < cycle->connection_n; i++) {
if (c[i].fd != -1
@@ -963,11 +963,11 @@ ngx_worker_process_exit(ngx_cycle_t *cyc
ngx_debug_quit = 1;
}
}
+}
-if (ngx_debug_quit) {
-ngx_log_error(NGX_LOG_ALERT, cycle->log, 0, "aborting");
-ngx_debug_point();
-}
+if (ngx_debug_quit) {
+ngx_log_error(NGX_LOG_ALERT, cycle->log, 0, "aborting");
+ngx_debug_point();
}
/*
diff --git a/src/os/win32/ngx_process_cycle.c b/src/os/win32/ngx_process_cycle.c
--- a/src/os/win32/ngx_process_cycle.c
+++ b/src/os/win32/ngx_process_cycle.c
@@ -834,7 +834,7 @@ ngx_worker_process_exit(ngx_cycle_t *cyc
}
}
-if (ngx_exiting) {
+if (ngx_exiting && !ngx_terminate) {
c = cycle->connections;
for (i = 0; i < cycle->connection_n; i++) {
if (c[i].fd != (ngx_socket_t) -1
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
[PATCH 0 of 4] aio fixes
Hello! Here is a patch series which fixes several issues with AIO (threaded IO) and some related issues, initially inspired by ticket #2555. Most notably, it fixes various issues with incorrect request termination with subrequests when an AIO operation is running in another subrequest, as observed in ticket #2555 (first patch). Additionally, it fixes an issue with request termination due to filter finalization with ngx_event_pipe() on stack (second patch), fixes incorrect alerts about open sockets left when SIGQUIT is followed by SIGTERM (third patch), and introduces guard timers for AIO operations to prevent premature shutdown of worker processes (forth patch, ticket #2162). In tests this fixes two alerts as observed with aio_write, in image_filter_finalize.t and in proxy_cache_use_stale.t: image_filter_finalize.t (Wstat: 0 Tests: 5 Failed: 0) TODO passed: 4 proxy_cache_use_stale.t (Wstat: 0 Tests: 36 Failed: 0) TODO passed: 35 Review and testing appreciated. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Linux packages: added Ubuntu 23.04 "mantic"
Hello! On Thu, Nov 09, 2023 at 07:43:09PM -0800, Konstantin Pavlov wrote: > # HG changeset patch > # User Konstantin Pavlov > # Date 1699587725 28800 > # Thu Nov 09 19:42:05 2023 -0800 > # Node ID d9dba9159ddf3adaf0263f17f3ed69228aa6c972 > # Parent 5cfaf094e2a041d3fa6eaf58799f575295e451ab > Linux packages: added Ubuntu 23.04 "mantic". > > diff -r 5cfaf094e2a0 -r d9dba9159ddf xml/en/linux_packages.xml > --- a/xml/en/linux_packages.xml Tue Oct 24 15:16:17 2023 -0700 > +++ b/xml/en/linux_packages.xml Thu Nov 09 19:42:05 2023 -0800 > @@ -7,7 +7,7 @@ > link="/en/linux_packages.html" > lang="en" > - rev="91"> > + rev="92"> > > > > @@ -92,6 +92,11 @@ versions: > x86_64, aarch64/arm64 > > > + > +23.10 “mantic” > +x86_64, aarch64/arm64 > + > + > > > > diff -r 5cfaf094e2a0 -r d9dba9159ddf xml/ru/linux_packages.xml > --- a/xml/ru/linux_packages.xml Tue Oct 24 15:16:17 2023 -0700 > +++ b/xml/ru/linux_packages.xml Thu Nov 09 19:42:05 2023 -0800 > @@ -7,7 +7,7 @@ > link="/ru/linux_packages.html" > lang="ru" > - rev="91"> > + rev="92"> > > > > @@ -92,6 +92,11 @@ > x86_64, aarch64/arm64 > > > + > +23.10 “mantic” > +x86_64, aarch64/arm64 > + > + > > Looks good. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: RESTCONF Server-Sent-Event session control in nginx
Hello! On Tue, Oct 31, 2023 at 06:20:43AM +, Lucas JunSeong Kim 김준성 wrote: > Hi, I have a question. > > We have the system which the following structure is used: > > Nginx <> CGI interpreter <> yumapro Netconf server > > Multiple CGI interpreters are created and used through the settings below. > > spawn-fcgi -s /var/run/fcgiwrap.socket -F 5 -- /var/www/yang-api/restconf > > Nginx <> CGI interpreter <> yumapro Netconf server > CGI interpreter > CGI interpreter > CGI interpreter > CGI interpreter [...] This mailing list is dedicated to nginx development. For questions on how to configure nginx, please use the nginx@ mailing list instead, see http://nginx.org/en/support.html for details. Thank you. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 2 of 2] HTTP: suppressed possible overflow in interim r->uri_end calculation
Hello!
On Fri, Oct 27, 2023 at 02:58:45PM +0300, Vladimir Homutov via nginx-devel
wrote:
> If URI is not fully parsed yet, the r->uri_end pointer is NULL.
> As a result, calculation of "new + (r->uri_end - old)" expression
> may overflow. In such case, just avoid calculating it, as r->uri_end
> will be set correctly later by the parser in any case.
>
> The issue was found by GCC undefined behaviour sanitizer.
>
>
> src/http/ngx_http_request.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
>
> # HG changeset patch
> # User Vladimir Khomutov
> # Date 1698407686 -10800
> # Fri Oct 27 14:54:46 2023 +0300
> # Node ID 1b28902de1c648fc2586bba8e05c2ff63e0e33cb
> # Parent ef9f124b156aff0e9f66057e438af835bd7a60d2
> HTTP: suppressed possible overflow in interim r->uri_end calculation.
>
> If URI is not fully parsed yet, the r->uri_end pointer is NULL.
> As a result, calculation of "new + (r->uri_end - old)" expression
> may overflow. In such case, just avoid calculating it, as r->uri_end
> will be set correctly later by the parser in any case.
>
> The issue was found by GCC undefined behaviour sanitizer.
>
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -1721,7 +1721,9 @@ ngx_http_alloc_large_header_buffer(ngx_h
> r->method_end = new + (r->method_end - old);
>
> r->uri_start = new + (r->uri_start - old);
> -r->uri_end = new + (r->uri_end - old);
> +if (r->uri_end) {
> +r->uri_end = new + (r->uri_end - old);
> +}
>
> if (r->schema_start) {
> r->schema_start = new + (r->schema_start - old);
As already noted off-list, this is certainly not the only field
which might be not yet set when
ngx_http_alloc_large_header_buffer() is called. From the patch
context as shown, at least r->method_end and r->uri_start might
not be set as well, leading to similar overflows. And certainly
there are other fields as well.
While I have no objections to fixing such overflows, which formally
might be seen as undefined behaviour (though safe in practice, since
calculated values are never used), I very much object to fixing
just the particular items which were reported by a particular
sanitizer in particular test runs.
Rather, sanitizer results should be used to identify patterns we
want to fix (if at all), and then all such patterns should be
fixed (or not).
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Linux packages: documented nginx-module-otel package
Hello! On Wed, Oct 25, 2023 at 10:52:40AM +0100, Yaroslav Zhuravlev wrote: [...] > As an optional variant to consider, perhaps it might be good > to reflect that it's a third party module authored by nginx devs, e.g: [...] > +Additionally, since version 1.25.3 > +nginx-authored third-party module > +https://github.com/nginxinc/nginx-otel;>nginx-otel > +is built as dynamic and shipped as a separate package: > + > +nginx-module-otel > + Note that "nginx-authored" here looks misleading, as no nginx core developers work on this module. Overall, I do support the clear distinction between nginx's own modules and 3rd-party modules provided in the packages repository. (But, as correctly noted by Konstantin, this should include njs as well.) -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: nginx 1.25.3 changes draft
Hello! On Tue, Oct 24, 2023 at 05:07:46PM +0400, Sergey Kandaurov wrote: > > > On 24 Oct 2023, at 16:34, Maxim Dounin wrote: > > > > Hello! > > > > > > Changes with nginx 1.25.324 Oct 2023 > > > >*) Change: improved detection of misbehaving clients when using HTTP/2. > > > >*) Feature: startup speedup when using a large number of locations. > > Thanks to Yusuke Nojima. > > > >*) Bugfix: a segmentation fault might occur in a worker process when > > using HTTP/2 without SSL; the bug had appeared in 1.25.1. > > > >*) Bugfix: the "Status" backend response header line with an empty > > reason phrase was handled incorrectly. > > > >*) Bugfix: memory leak during reconfiguration when using the PCRE2 > > library. > > Thanks to ZhenZhong Wu. > > > >*) Bugfixes and improvements in HTTP/3. > > > > > > Изменения в nginx 1.25.3 24.10.2023 > > > >*) Изменение: улучшено детектирование некорректного поведения клиентов > > при использовании HTTP/2. > > > >*) Добавление: уменьшение времени запуска при использовании большого > > количества location'ов. > > Спасибо Yusuke Nojima. > > > >*) Исправление: при использовании HTTP/2 без SSL в рабочем процессе мог > > произойти segmentation fault; ошибка появилась в 1.25.1. > > > >*) Исправление: строка "Status" в заголовке ответа бэкенда с пустой > > поясняющей фразой обрабатывалась некорректно. > > > >*) Исправление: утечки памяти во время переконфигурации при > > использовании библиотеки PCRE2. > > Спасибо ZhenZhong Wu. > > > >*) Исправления и улучшения в HTTP/3. > > > > > > Looks good. Thanks, pushed: http://mdounin.ru/hg/nginx http://mdounin.ru/hg/nginx.org Release files: http://mdounin.ru/temp/nginx-1.25.3.tar.gz http://mdounin.ru/temp/nginx-1.25.3.tar.gz.asc http://mdounin.ru/temp/nginx-1.25.3.zip http://mdounin.ru/temp/nginx-1.25.3.zip.asc -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
nginx 1.25.3 changes draft
Hello! Changes with nginx 1.25.324 Oct 2023 *) Change: improved detection of misbehaving clients when using HTTP/2. *) Feature: startup speedup when using a large number of locations. Thanks to Yusuke Nojima. *) Bugfix: a segmentation fault might occur in a worker process when using HTTP/2 without SSL; the bug had appeared in 1.25.1. *) Bugfix: the "Status" backend response header line with an empty reason phrase was handled incorrectly. *) Bugfix: memory leak during reconfiguration when using the PCRE2 library. Thanks to ZhenZhong Wu. *) Bugfixes and improvements in HTTP/3. Изменения в nginx 1.25.3 24.10.2023 *) Изменение: улучшено детектирование некорректного поведения клиентов при использовании HTTP/2. *) Добавление: уменьшение времени запуска при использовании большого количества location'ов. Спасибо Yusuke Nojima. *) Исправление: при использовании HTTP/2 без SSL в рабочем процессе мог произойти segmentation fault; ошибка появилась в 1.25.1. *) Исправление: строка "Status" в заголовке ответа бэкенда с пустой поясняющей фразой обрабатывалась некорректно. *) Исправление: утечки памяти во время переконфигурации при использовании библиотеки PCRE2. Спасибо ZhenZhong Wu. *) Исправления и улучшения в HTTP/3. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] HTTP/2: fixed buffer management with HTTP/2 auto-detection
Hello!
On Fri, Oct 20, 2023 at 06:04:32PM +0400, Sergey Kandaurov wrote:
> > On 20 Oct 2023, at 17:23, Sergey Kandaurov wrote:
> >
> > # HG changeset patch
> > # User Sergey Kandaurov
> > # Date 1697808142 -14400
> > # Fri Oct 20 17:22:22 2023 +0400
> > # Node ID 318c8ace6aa24506004bfbb7d52674f61a3716a5
> > # Parent 3038bd4d78169a5e8a2624d79cf76f45f0805ddc
> > HTTP/2: fixed buffer management with HTTP/2 auto-detection.
> >
> > As part of normal HTTP/2 processing, incomplete frames are saved in the
> > control state using a fixed size memcpy of NGX_HTTP_V2_STATE_BUFFER_SIZE.
> > For this matter, two state buffers are reserved in the HTTP/2 recv buffer.
> >
> > As part of HTTP/2 auto-detection on plain TCP connections, initial data
> > is first read into a buffer specified by the client_header_buffer_size
> > directive that doesn't have state reservation. Previously, this made it
> > possible to over-read the buffer as part of saving the state.
> >
> > The fix is to read the available buffer size rather than a fixed size.
> > Although memcpy of a fixed size can produce a better optimized code,
>
> From my limited testing, replacing a fixed size with an available size
> degrades "-O" optimized memcpy from SSE instructions over XMM registers
> to simple MOVs.
I don't think it matters compared to other costs within the loop.
> > handling of incomplete frames isn't a common execution path, so it was
> > sacrificed for the sake of simplicity of the fix.
>
> Another approach is to displace initial data into the recv buffer
> for subsequent processing, which would require additional handling
> in ngx_http_v2_init(). After some pondering I declined it due to
> added complexity without a good reason.
>
> >
> > diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
> > --- a/src/http/v2/ngx_http_v2.c
> > +++ b/src/http/v2/ngx_http_v2.c
> > @@ -386,13 +386,11 @@ ngx_http_v2_read_handler(ngx_event_t *re
> > h2mcf = ngx_http_get_module_main_conf(h2c->http_connection->conf_ctx,
> > ngx_http_v2_module);
> >
> > -available = h2mcf->recv_buffer_size - 2 *
> > NGX_HTTP_V2_STATE_BUFFER_SIZE;
> > +available = h2mcf->recv_buffer_size - NGX_HTTP_V2_STATE_BUFFER_SIZE;
> >
> > do {
> > p = h2mcf->recv_buffer;
> > -
> > -ngx_memcpy(p, h2c->state.buffer, NGX_HTTP_V2_STATE_BUFFER_SIZE);
> > -end = p + h2c->state.buffer_used;
> > +end = ngx_cpymem(p, h2c->state.buffer, h2c->state.buffer_used);
> >
> > n = c->recv(c, end, available);
> >
> > @@ -2592,7 +2590,7 @@ ngx_http_v2_state_save(ngx_http_v2_conne
> > return ngx_http_v2_connection_error(h2c,
> > NGX_HTTP_V2_INTERNAL_ERROR);
> > }
> >
> > -ngx_memcpy(h2c->state.buffer, pos, NGX_HTTP_V2_STATE_BUFFER_SIZE);
> > +ngx_memcpy(h2c->state.buffer, pos, size);
> >
> > h2c->state.buffer_used = size;
> > h2c->state.handler = handler;
> > diff --git a/src/http/v2/ngx_http_v2_module.c
> > b/src/http/v2/ngx_http_v2_module.c
> > --- a/src/http/v2/ngx_http_v2_module.c
> > +++ b/src/http/v2/ngx_http_v2_module.c
> > @@ -388,7 +388,7 @@ ngx_http_v2_recv_buffer_size(ngx_conf_t
> > {
> > size_t *sp = data;
> >
> > -if (*sp <= 2 * NGX_HTTP_V2_STATE_BUFFER_SIZE) {
> > +if (*sp <= NGX_HTTP_V2_STATE_BUFFER_SIZE) {
> > return "value is too small";
> > }
> >
Looks good.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Improve performance when starting nginx with a lot of locations
Hello! On Tue, Oct 17, 2023 at 03:25:41PM +0400, Sergey Kandaurov wrote: > > On 11 Oct 2023, at 02:56, Maxim Dounin wrote: > > > > Hello! > > > > On Thu, Oct 05, 2023 at 10:51:26AM +0900, Yusuke Nojima wrote: > > > >> Thank you for your comment! > >> > >>> Could you please provide some more details about the use case, > >>> such as how locations are used, and what is the approximate number > >>> of locations being used? > >> > >> Our team provides development environments to our company's engineers and > >> QA. > >> In this environment, engineers and QA can freely create VMs and deploy > >> applications on them. > >> > >> Our nginx has the role of routing requests from the internet to all > >> applications deployed in this environment. > >> Additionally, it allows setting IP address restrictions, BASIC > >> authentication, TLS client authentication, and other configurations > >> for each application. > >> > >> To implement these requirements, we generate a location for each > >> application. > >> Currently, there are approximately 40,000 locations in our environment. > > > > Thank you for the details. Such configuration looks somewhat > > sub-optimal, but understandable for a development / test > > environment. And certainly 40k locations is a lot for the sorting > > algorithm currently used. > > > >>> Further, since each location contains configuration for > >>> all modules, such configurations are expected to require a lot of > >>> memory > >> > >> Each of our nginx processes was consuming 5GB of memory in terms of > >> resident size. > >> This is not a problem as our servers have sufficient memory. > >> > >>> Rather, I would suggest recursive top-bottom merge sort implementation > >>> instead, which is much simpler and uses stack as temporary storage > >>> (so it'll naturally die if there will be a queue which requires > >>> more space for sorting than we have). > > For the record, in my tests on M1 sorting 26^3 locations fit into > 32k stack size (16k stack size renders the environment unusable). > Judging by this (unscientific) test, running out of stack should > not be a practicable issue. The recursive function uses ngx_queue_t *q, tail; on stack, that is, 3 pointers, so it would need more than 1000 recursive calls to overflow 32k stack, which is not expected to happen unless there are more than 2^1000 locations (and it is certainly not possible to have more than 2^64 locations on 64-bit platforms). For 26^3 locations, sorting would use maximum recursion depth of 15, so would require something like 360 bytes of stack (which is certainly less than required by other nginx functions). > >>> > >>> Please take a look if it works for you: > >> > >> I think this implementation is simple and easy to understand. > >> Although the number of traversals of the list will increase compared > >> to bottom-up, it will not affect the order. > >> I believe this will provide sufficient optimization in terms of speed. > > > > Thanks for looking. In my limited testing, it is slightly faster > > than your bottom-up implementation (and significantly faster than > > the existing insertion sort when many locations are used). > > > > Below is the full patch (code unchanged), I'll commit it as soon > > as some other nginx developer will review it. > > > > # HG changeset patch > > # User Maxim Dounin > > # Date 1696977468 -10800 > > # Wed Oct 11 01:37:48 2023 +0300 > > # Node ID b891840852ee5cc823eee1769d092ab50928919f > > # Parent cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc > > Core: changed ngx_queue_sort() to use merge sort. > > > > This improves nginx startup times significantly when using very large number > > of locations due computational complexity of the sorting algorithm being > > due to Fixed, thnx. > > used (insertion sort is O(n*n) on average, while merge sort is O(n*log(n))). > > nitpicking: E2MANYPARENS > > Using a colon might looks better (I don't mind though): > used: insertion sort is O(n*n) on average, while merge sort is O(n*log(n)). Thanks, changed. > > In particular, in a test configuration with 20k locations total startup > > time is reduced from 8 seconds to 0.9 seconds. > > > > Prodded by Yusuke Nojima, > > https://mailman.nginx.org/pipermail/nginx-devel/2023-September/NUL3Y2FPPFSHMPTFTL65KXSXNTX3NQMK.htm
Re: Memory Leak Issue in Nginx PCRE2
Hello!
On Mon, Oct 16, 2023 at 06:19:48PM +0400, Sergey Kandaurov wrote:
>
> > On 11 Oct 2023, at 02:04, Maxim Dounin wrote:
> >
> > Hello!
> >
> > On Wed, Sep 27, 2023 at 01:13:44AM +0800, 上勾拳 wrote:
> >
> >> Dear Nginx Developers,
> >>
> >> I hope this email finds you well. I am reaching out to the mailing list for
> >> the first time to report and discuss an issue I encountered while working
> >> on supporting PCRE2 in OpenResty. If I have made any errors in my reporting
> >> or discussion, please do not hesitate to provide feedback. Your guidance is
> >> greatly appreciated.
> >>
> >> During my recent work, I used the sanitizer to inspect potential issues,
> >> and I identified a small memory leak in the PCRE2 code section of Nginx.
> >> While this issue does not seem to be critical, it could potentially disrupt
> >> memory checking tools. To help you reproduce the problem, I have included a
> >> minimal configuration below. Please note that this issue occurs when Nginx
> >> is configured to use PCRE2, and the version is 1.22.1 or higher.
> >>
> >> *Minimal Configuration for Reproduction:*
> >> worker_processes 1;
> >> daemon off;
> >> master_process off;
> >> error_log
> >> /home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/servroot/logs/error.log
> >> debug;
> >> pid
> >> /home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/servroot/logs/nginx.pid;
> >>
> >> http {
> >>access_log
> >> /home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/servroot/logs/access.log;
> >>#access_log off;
> >>default_type text/plain;
> >>keepalive_timeout 68000ms;
> >>server {
> >>listen 1984;
> >>#placeholder
> >>server_name 'localhost';
> >>
> >>client_max_body_size 30M;
> >>#client_body_buffer_size 4k;
> >>
> >># Begin preamble config...
> >>
> >># End preamble config...
> >>
> >># Begin test case config...
> >>
> >>location ~ '^/[a-d]$' {
> >>return 200;
> >>}
> >>}
> >> }
> >> events {
> >>accept_mutex off;
> >>
> >>worker_connections 64;
> >> }
> >>
> >> *nginx -V :*
> >> nginx version: nginx/1.25.1 (no pool)
> >> built by gcc 11.4.1 20230605 (Red Hat 11.4.1-2) (GCC)
> >> built with OpenSSL 1.1.1u 30 May 2023
> >> TLS SNI support enabled
> >> configure arguments:
> >> --prefix=/home/zhenzhongw/code/pcre_pr/lua-nginx-module/work/nginx
> >> --with-threads --with-pcre-jit --with-ipv6
> >> --with-cc-opt='-fno-omit-frame-pointer -fsanitize=address
> >> -DNGX_LUA_USE_ASSERT -I/opt/pcre2/include -I/opt/ssl/include'
> >> --with-http_v2_module --with-http_v3_module --with-http_realip_module
> >> --with-http_ssl_module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/ndk-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/set-misc-nginx-module
> >> --with-ld-opt='-fsanitize=address -L/opt/pcre2/lib -L/opt/ssl/lib
> >> -Wl,-rpath,/opt/pcre2/lib:/opt/drizzle/lib:/opt/ssl/lib'
> >> --without-mail_pop3_module --without-mail_imap_module
> >> --with-http_image_filter_module --without-mail_smtp_module --with-stream
> >> --with-stream_ssl_module --without-http_upstream_ip_hash_module
> >> --without-http_memcached_module --without-http_auth_basic_module
> >> --without-http_userid_module --with-http_auth_request_module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/echo-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/memc-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/srcache-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/lua-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/lua-upstream-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/headers-more-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/drizzle-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/rds-json-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/coolkit-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/redis2-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/stream-lua-nginx-module
> >> --add-module=/home/zhenzhongw/code/pcre_pr/lua-nginx-module/t/data/f
Re: [PATCH] Improve performance when starting nginx with a lot of locations
Hello!
On Thu, Oct 05, 2023 at 10:51:26AM +0900, Yusuke Nojima wrote:
> Thank you for your comment!
>
> > Could you please provide some more details about the use case,
> > such as how locations are used, and what is the approximate number
> > of locations being used?
>
> Our team provides development environments to our company's engineers and QA.
> In this environment, engineers and QA can freely create VMs and deploy
> applications on them.
>
> Our nginx has the role of routing requests from the internet to all
> applications deployed in this environment.
> Additionally, it allows setting IP address restrictions, BASIC
> authentication, TLS client authentication, and other configurations
> for each application.
>
> To implement these requirements, we generate a location for each application.
> Currently, there are approximately 40,000 locations in our environment.
Thank you for the details. Such configuration looks somewhat
sub-optimal, but understandable for a development / test
environment. And certainly 40k locations is a lot for the sorting
algorithm currently used.
> > Further, since each location contains configuration for
> > all modules, such configurations are expected to require a lot of
> > memory
>
> Each of our nginx processes was consuming 5GB of memory in terms of
> resident size.
> This is not a problem as our servers have sufficient memory.
>
> > Rather, I would suggest recursive top-bottom merge sort implementation
> > instead, which is much simpler and uses stack as temporary storage
> > (so it'll naturally die if there will be a queue which requires
> > more space for sorting than we have).
> >
> > Please take a look if it works for you:
>
> I think this implementation is simple and easy to understand.
> Although the number of traversals of the list will increase compared
> to bottom-up, it will not affect the order.
> I believe this will provide sufficient optimization in terms of speed.
Thanks for looking. In my limited testing, it is slightly faster
than your bottom-up implementation (and significantly faster than
the existing insertion sort when many locations are used).
Below is the full patch (code unchanged), I'll commit it as soon
as some other nginx developer will review it.
# HG changeset patch
# User Maxim Dounin
# Date 1696977468 -10800
# Wed Oct 11 01:37:48 2023 +0300
# Node ID b891840852ee5cc823eee1769d092ab50928919f
# Parent cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc
Core: changed ngx_queue_sort() to use merge sort.
This improves nginx startup times significantly when using very large number
of locations due computational complexity of the sorting algorithm being
used (insertion sort is O(n*n) on average, while merge sort is O(n*log(n))).
In particular, in a test configuration with 20k locations total startup
time is reduced from 8 seconds to 0.9 seconds.
Prodded by Yusuke Nojima,
https://mailman.nginx.org/pipermail/nginx-devel/2023-September/NUL3Y2FPPFSHMPTFTL65KXSXNTX3NQMK.html
diff --git a/src/core/ngx_queue.c b/src/core/ngx_queue.c
--- a/src/core/ngx_queue.c
+++ b/src/core/ngx_queue.c
@@ -9,6 +9,10 @@
#include
+static void ngx_queue_merge(ngx_queue_t *queue, ngx_queue_t *tail,
+ngx_int_t (*cmp)(const ngx_queue_t *, const ngx_queue_t *));
+
+
/*
* find the middle queue element if the queue has odd number of elements
* or the first element of the queue's second part otherwise
@@ -45,13 +49,13 @@ ngx_queue_middle(ngx_queue_t *queue)
}
-/* the stable insertion sort */
+/* the stable merge sort */
void
ngx_queue_sort(ngx_queue_t *queue,
ngx_int_t (*cmp)(const ngx_queue_t *, const ngx_queue_t *))
{
-ngx_queue_t *q, *prev, *next;
+ngx_queue_t *q, tail;
q = ngx_queue_head(queue);
@@ -59,22 +63,44 @@ ngx_queue_sort(ngx_queue_t *queue,
return;
}
-for (q = ngx_queue_next(q); q != ngx_queue_sentinel(queue); q = next) {
+q = ngx_queue_middle(queue);
+
+ngx_queue_split(queue, q, );
+
+ngx_queue_sort(queue, cmp);
+ngx_queue_sort(, cmp);
+
+ngx_queue_merge(queue, , cmp);
+}
-prev = ngx_queue_prev(q);
-next = ngx_queue_next(q);
-ngx_queue_remove(q);
+static void
+ngx_queue_merge(ngx_queue_t *queue, ngx_queue_t *tail,
+ngx_int_t (*cmp)(const ngx_queue_t *, const ngx_queue_t *))
+{
+ngx_queue_t *q1, *q2;
+
+q1 = ngx_queue_head(queue);
+q2 = ngx_queue_head(tail);
-do {
-if (cmp(prev, q) <= 0) {
-break;
-}
+for ( ;; ) {
+if (q1 == ngx_queue_sentinel(queue)) {
+ngx_queue_add(queue, tail);
+break;
+}
+
+if (q2 == ngx_queue_sentinel(tail)) {
+break;
+}
-prev = ngx_queue_prev(prev);
+if (cmp(q1, q2) <= 0) {
+
Re: Memory Leak Issue in Nginx PCRE2
bles.c:2555
> #5 0x536088 in ngx_http_core_regex_location
> src/http/ngx_http_core_module.c:3263
> #6 0x537f94 in ngx_http_core_location
> src/http/ngx_http_core_module.c:3115
> #7 0x46ba0a in ngx_conf_handler src/core/ngx_conf_file.c:463
> #8 0x46ba0a in ngx_conf_parse src/core/ngx_conf_file.c:319
> #9 0x5391ec in ngx_http_core_server src/http/ngx_http_core_module.c:2991
> #10 0x46ba0a in ngx_conf_handler src/core/ngx_conf_file.c:463
> #11 0x46ba0a in ngx_conf_parse src/core/ngx_conf_file.c:319
> #12 0x528e4c in ngx_http_block src/http/ngx_http.c:239
> #13 0x46ba0a in ngx_conf_handler src/core/ngx_conf_file.c:463
> #14 0x46ba0a in ngx_conf_parse src/core/ngx_conf_file.c:319
> #15 0x463f74 in ngx_init_cycle src/core/ngx_cycle.c:284
> #12 0x528e4c in ngx_http_block src/http/ngx_http.c:239
> #13 0x46ba0a in ngx_conf_handler src/core/ngx_conf_file.c:463
> #14 0x46ba0a in ngx_conf_parse src/core/ngx_conf_file.c:319
> #15 0x463f74 in ngx_init_cycle src/core/ngx_cycle.c:284
> #16 0x4300c7 in main src/core/nginx.c:295
> #17 0x7ff31a43feaf in __libc_start_call_main (/lib64/libc.so.6+0x3feaf)
>
> SUMMARY: AddressSanitizer: 72 byte(s) leaked in 1 allocation(s).
>
> *I have created a patch to address this memory leak issue, which I am
> sharing below:*
> diff --git a/src/core/ngx_regex.c b/src/core/ngx_regex.c
> index 91381f499..71f583789 100644
> --- a/src/core/ngx_regex.c
> +++ b/src/core/ngx_regex.c
> @@ -600,6 +600,8 @@ ngx_regex_cleanup(void *data)
> * the new cycle, these will be re-allocated.
> */
>
> +ngx_regex_malloc_init(NULL);
> +
> if (ngx_regex_compile_context) {
> pcre2_compile_context_free(ngx_regex_compile_context);
> ngx_regex_compile_context = NULL;
> @@ -611,6 +613,8 @@ ngx_regex_cleanup(void *data)
> ngx_regex_match_data_size = 0;
> }
>
> +ngx_regex_malloc_done();
> +
> #endif
> }
>
> @@ -706,7 +710,13 @@ ngx_regex_module_init(ngx_cycle_t *cycle)
> ngx_regex_malloc_done();
>
> ngx_regex_studies = NULL;
> +
> #if (NGX_PCRE2)
> +if (ngx_regex_compile_context) {
> +ngx_regex_malloc_init(NULL);
> +pcre2_compile_context_free(ngx_regex_compile_context);
> +ngx_regex_malloc_done();
> +}
> ngx_regex_compile_context = NULL;
> #endif
>
> I kindly request your assistance in reviewing this matter and considering
> the patch for inclusion in Nginx. If you have any questions or need further
> information, please feel free to reach out to me. Your expertise and
> feedback are highly valuable in resolving this issue.
Thank you for the report.
Indeed, this looks like a small leak which manifests itself during
reconfiguration when nginx is compiled with PCRE2.
The patch looks correct to me, though I think it would be better
to don't do anything with ngx_regex_compile_context in
ngx_regex_module_init(). Please take a look if the following
patch looks good to you:
# HG changeset patch
# User Maxim Dounin
# Date 1696950530 -10800
# Tue Oct 10 18:08:50 2023 +0300
# Node ID 0ceb96f57592b77618fba4200797df977241ec9b
# Parent cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc
Core: fixed memory leak on configuration reload with PCRE2.
In ngx_regex_cleanup() allocator wasn't configured when calling
pcre2_compile_context_free() and pcre2_match_data_free(), resulting
in no ngx_free() call and leaked memory. Fix is ensure that allocator
is configured for global allocations, so that ngx_free() is actually
called to free memory.
Additionally, ngx_regex_compile_context was cleared in
ngx_regex_module_init(). It should be either not cleared, so it will
be freed by ngx_regex_cleanup(), or properly freed. Fix is to
not clear it, so ngx_regex_cleanup() will be able to free it.
Reported by ZhenZhong Wu,
https://mailman.nginx.org/pipermail/nginx-devel/2023-September/3Z5FIKUDRN2WBSL3JWTZJ7SXDA6YIWPB.html
diff --git a/src/core/ngx_regex.c b/src/core/ngx_regex.c
--- a/src/core/ngx_regex.c
+++ b/src/core/ngx_regex.c
@@ -600,6 +600,8 @@ ngx_regex_cleanup(void *data)
* the new cycle, these will be re-allocated.
*/
+ngx_regex_malloc_init(NULL);
+
if (ngx_regex_compile_context) {
pcre2_compile_context_free(ngx_regex_compile_context);
ngx_regex_compile_context = NULL;
@@ -611,6 +613,8 @@ ngx_regex_cleanup(void *data)
ngx_regex_match_data_size = 0;
}
+ngx_regex_malloc_done();
+
#endif
}
@@ -706,9 +710,6 @@ ngx_regex_module_init(ngx_cycle_t *cycle
ngx_regex_malloc_done();
ngx_regex_studies = NULL;
-#if (NGX_PCRE2)
-ngx_regex_compile_context = NULL;
-#endif
return NGX_OK;
}
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] HTTP/2: per-iteration stream handling limit
Hello! On Tue, Oct 10, 2023 at 05:57:58PM +, Thomas Ward wrote: > Maxim: > > If NGINX is unaffected, then F5 is distributing the FUD. Yes, they are. > Refer to https://my.f5.com/manage/s/article/K000137106 and the > NGINX category, specifically NGINX OSS. Right now, unless this > is updated by F5 (who owns NGINX now) there is conflicting > information here. May I suggest you go internally to F5 and > have them actually *revise* their update, or further indicate > that "There are already mitigations in place"? I'm not "internally to F5", I'm an independent developer. This was discussed within nginx security team, with other nginx developers, and the conclusion is that nginx is not affected. I have no idea why F5 decided to include nginx as affected in their advisory, who did this, and why. This is incorrect and should be fixed. I've already pinged them, and hopefully they will fix this. > Ultimately, > though, I would assume that keepalive at 100 is also still not > enough to *fully* protect against this, therefore at the core > "NGINX is still vulnerable to this CVE despite mitigations > already being in place for default configuration values and > options" is the interpretation, which means yes NGINX is in fact > affected by this *in non-default configurations*, which means > that it *is* still vulnerable to the CVE. Mitigations don't > mean "fixed" or "not affected" in the strictest interpretation > of languages and what means what. As often the case for DoS vulnerabilities, you cannot be *fully* protected, since there is a work to do anyway. As long as nginx is configured to serve HTTP requests, it can be loaded with serving HTTP requests, and maximum possible load depends on the configurations, including various builtin limits, such as keepalive_requests and http2_max_concurrent_streams, and DoS mitigations which needs to be explicitly configured, such as limit_req and limit_conn. As already said, in this case the work nginx is willing to do is no different from other workloads, such as with normal HTTP/2 requests or HTTP/1.x requests. As such, nginx is not considered to be affected by this issue. > > > Thomas > > -Original Message- > From: nginx-devel On Behalf Of Maxim Dounin > Sent: Tuesday, October 10, 2023 8:53 AM > To: nginx-devel@nginx.org > Subject: Re: [PATCH] HTTP/2: per-iteration stream handling limit > > Hello! > > On Tue, Oct 10, 2023 at 03:29:02PM +0300, Maxim Dounin wrote: > > > # HG changeset patch > > # User Maxim Dounin # Date 1696940019 -10800 > > # Tue Oct 10 15:13:39 2023 +0300 > > # Node ID cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc > > # Parent 3db945fda515014d220151046d02f3960bcfca0a > > HTTP/2: per-iteration stream handling limit. > > > > To ensure that attempts to flood servers with many streams are > > detected early, a limit of no more than 2 * max_concurrent_streams new > > streams per one event loop iteration was introduced. This limit is > > applied even if max_concurrent_streams is not yet reached - for > > example, if corresponding streams are handled synchronously or reset. > > > > Further, refused streams are now limited to maximum of > > max_concurrent_streams and 100, similarly to priority_limit initial > > value, providing some tolerance to clients trying to open several > > streams at the connection start, yet low tolerance to flooding attempts. > > To clarify: > > There is FUD being spread that nginx is vulnerable to CVE-2023-44487. > > We do not consider nginx to be affected by this issue. In the default > configuration, nginx is sufficiently protected by the limit of allowed > requests per connection (see http://nginx.org/r/keepalive_requests for > details), so an attacker will be required to reconnect very often, making the > attack obvious and easy to stop at the network level. And it is not possible > to circumvent the max concurrent streams limit in nginx, as nginx only allows > additional streams when previous streams are completely closed. > > Further, additional protection can be implemented in nginx by using the > "limit_req" directive, which limits the rate of requests and rejects > excessive requests. > > Overall, with the handling as implemented in nginx, impact of streams being > reset does no seem to be significantly different from impacts from over > workloads with large number of requests being sent by the client, such as > handling of multiple HTTP/2 requests or HTTP/1.x pipelined requests. > > Nevertheless, we've decided to implemented some additional mitigations which > will help nginx to detect such attacks and
Re: [PATCH] HTTP/2: per-iteration stream handling limit
Hello!
On Tue, Oct 10, 2023 at 04:46:04PM +0400, Sergey Kandaurov wrote:
>
> > On 10 Oct 2023, at 16:29, Maxim Dounin wrote:
> >
> > # HG changeset patch
> > # User Maxim Dounin
> > # Date 1696940019 -10800
> > # Tue Oct 10 15:13:39 2023 +0300
> > # Node ID cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc
> > # Parent 3db945fda515014d220151046d02f3960bcfca0a
> > HTTP/2: per-iteration stream handling limit.
> >
> > To ensure that attempts to flood servers with many streams are detected
> > early, a limit of no more than 2 * max_concurrent_streams new streams per
> > one
> > event loop iteration was introduced. This limit is applied even if
> > max_concurrent_streams is not yet reached - for example, if corresponding
> > streams are handled synchronously or reset.
> >
> > Further, refused streams are now limited to maximum of
> > max_concurrent_streams
> > and 100, similarly to priority_limit initial value, providing some tolerance
> > to clients trying to open several streams at the connection start, yet
> > low tolerance to flooding attempts.
> >
> > diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
> > --- a/src/http/v2/ngx_http_v2.c
> > +++ b/src/http/v2/ngx_http_v2.c
> > @@ -347,6 +347,7 @@ ngx_http_v2_read_handler(ngx_event_t *re
> > ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http2 read handler");
> >
> > h2c->blocked = 1;
> > +h2c->new_streams = 0;
> >
> > if (c->close) {
> > c->close = 0;
> > @@ -1284,6 +1285,14 @@ ngx_http_v2_state_headers(ngx_http_v2_co
> > goto rst_stream;
> > }
> >
> > +if (h2c->new_streams++ >= 2 * h2scf->concurrent_streams) {
> > +ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> > + "client sent too many streams at once");
> > +
> > +status = NGX_HTTP_V2_REFUSED_STREAM;
> > +goto rst_stream;
> > +}
> > +
> > if (!h2c->settings_ack
> > && !(h2c->state.flags & NGX_HTTP_V2_END_STREAM_FLAG)
> > && h2scf->preread_size < NGX_HTTP_V2_DEFAULT_WINDOW)
> > @@ -1349,6 +1358,12 @@ ngx_http_v2_state_headers(ngx_http_v2_co
> >
> > rst_stream:
> >
> > +if (h2c->refused_streams++ > ngx_max(h2scf->concurrent_streams, 100)) {
> > +ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> > + "client sent too many refused streams");
> > +return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_NO_ERROR);
> > +}
> > +
> > if (ngx_http_v2_send_rst_stream(h2c, h2c->state.sid, status) != NGX_OK)
> > {
> > return ngx_http_v2_connection_error(h2c,
> > NGX_HTTP_V2_INTERNAL_ERROR);
> > }
> > diff --git a/src/http/v2/ngx_http_v2.h b/src/http/v2/ngx_http_v2.h
> > --- a/src/http/v2/ngx_http_v2.h
> > +++ b/src/http/v2/ngx_http_v2.h
> > @@ -131,6 +131,8 @@ struct ngx_http_v2_connection_s {
> > ngx_uint_t processing;
> > ngx_uint_t frames;
> > ngx_uint_t idle;
> > +ngx_uint_t new_streams;
> > +ngx_uint_t refused_streams;
> > ngx_uint_t priority_limit;
> >
> > size_t send_window;
>
> Looks good.
Pushed to http://mdounin.ru/hg/nginx/, thanks.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] HTTP/2: per-iteration stream handling limit
Hello!
On Tue, Oct 10, 2023 at 03:29:02PM +0300, Maxim Dounin wrote:
> # HG changeset patch
> # User Maxim Dounin
> # Date 1696940019 -10800
> # Tue Oct 10 15:13:39 2023 +0300
> # Node ID cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc
> # Parent 3db945fda515014d220151046d02f3960bcfca0a
> HTTP/2: per-iteration stream handling limit.
>
> To ensure that attempts to flood servers with many streams are detected
> early, a limit of no more than 2 * max_concurrent_streams new streams per one
> event loop iteration was introduced. This limit is applied even if
> max_concurrent_streams is not yet reached - for example, if corresponding
> streams are handled synchronously or reset.
>
> Further, refused streams are now limited to maximum of max_concurrent_streams
> and 100, similarly to priority_limit initial value, providing some tolerance
> to clients trying to open several streams at the connection start, yet
> low tolerance to flooding attempts.
To clarify:
There is FUD being spread that nginx is vulnerable to
CVE-2023-44487.
We do not consider nginx to be affected by this issue. In the
default configuration, nginx is sufficiently protected by the
limit of allowed requests per connection (see
http://nginx.org/r/keepalive_requests for details), so an attacker
will be required to reconnect very often, making the attack
obvious and easy to stop at the network level. And it is not
possible to circumvent the max concurrent streams limit in nginx,
as nginx only allows additional streams when previous streams are
completely closed.
Further, additional protection can be implemented in nginx by
using the "limit_req" directive, which limits the rate of requests
and rejects excessive requests.
Overall, with the handling as implemented in nginx, impact of
streams being reset does no seem to be significantly different
from impacts from over workloads with large number of requests
being sent by the client, such as handling of multiple HTTP/2
requests or HTTP/1.x pipelined requests.
Nevertheless, we've decided to implemented some additional
mitigations which will help nginx to detect such attacks and drop
connections with misbehaving clients faster. Hence the patch.
>
> diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c
> +++ b/src/http/v2/ngx_http_v2.c
> @@ -347,6 +347,7 @@ ngx_http_v2_read_handler(ngx_event_t *re
> ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http2 read handler");
>
> h2c->blocked = 1;
> +h2c->new_streams = 0;
>
> if (c->close) {
> c->close = 0;
> @@ -1284,6 +1285,14 @@ ngx_http_v2_state_headers(ngx_http_v2_co
> goto rst_stream;
> }
>
> +if (h2c->new_streams++ >= 2 * h2scf->concurrent_streams) {
> +ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> + "client sent too many streams at once");
> +
> +status = NGX_HTTP_V2_REFUSED_STREAM;
> +goto rst_stream;
> +}
> +
> if (!h2c->settings_ack
> && !(h2c->state.flags & NGX_HTTP_V2_END_STREAM_FLAG)
> && h2scf->preread_size < NGX_HTTP_V2_DEFAULT_WINDOW)
> @@ -1349,6 +1358,12 @@ ngx_http_v2_state_headers(ngx_http_v2_co
>
> rst_stream:
>
> +if (h2c->refused_streams++ > ngx_max(h2scf->concurrent_streams, 100)) {
> +ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> + "client sent too many refused streams");
> +return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_NO_ERROR);
> +}
> +
> if (ngx_http_v2_send_rst_stream(h2c, h2c->state.sid, status) != NGX_OK) {
> return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_INTERNAL_ERROR);
> }
> diff --git a/src/http/v2/ngx_http_v2.h b/src/http/v2/ngx_http_v2.h
> --- a/src/http/v2/ngx_http_v2.h
> +++ b/src/http/v2/ngx_http_v2.h
> @@ -131,6 +131,8 @@ struct ngx_http_v2_connection_s {
> ngx_uint_t processing;
> ngx_uint_t frames;
> ngx_uint_t idle;
> +ngx_uint_t new_streams;
> +ngx_uint_t refused_streams;
> ngx_uint_t priority_limit;
>
> size_t send_window;
> ___
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
[PATCH] HTTP/2: per-iteration stream handling limit
# HG changeset patch
# User Maxim Dounin
# Date 1696940019 -10800
# Tue Oct 10 15:13:39 2023 +0300
# Node ID cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc
# Parent 3db945fda515014d220151046d02f3960bcfca0a
HTTP/2: per-iteration stream handling limit.
To ensure that attempts to flood servers with many streams are detected
early, a limit of no more than 2 * max_concurrent_streams new streams per one
event loop iteration was introduced. This limit is applied even if
max_concurrent_streams is not yet reached - for example, if corresponding
streams are handled synchronously or reset.
Further, refused streams are now limited to maximum of max_concurrent_streams
and 100, similarly to priority_limit initial value, providing some tolerance
to clients trying to open several streams at the connection start, yet
low tolerance to flooding attempts.
diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
--- a/src/http/v2/ngx_http_v2.c
+++ b/src/http/v2/ngx_http_v2.c
@@ -347,6 +347,7 @@ ngx_http_v2_read_handler(ngx_event_t *re
ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http2 read handler");
h2c->blocked = 1;
+h2c->new_streams = 0;
if (c->close) {
c->close = 0;
@@ -1284,6 +1285,14 @@ ngx_http_v2_state_headers(ngx_http_v2_co
goto rst_stream;
}
+if (h2c->new_streams++ >= 2 * h2scf->concurrent_streams) {
+ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
+ "client sent too many streams at once");
+
+status = NGX_HTTP_V2_REFUSED_STREAM;
+goto rst_stream;
+}
+
if (!h2c->settings_ack
&& !(h2c->state.flags & NGX_HTTP_V2_END_STREAM_FLAG)
&& h2scf->preread_size < NGX_HTTP_V2_DEFAULT_WINDOW)
@@ -1349,6 +1358,12 @@ ngx_http_v2_state_headers(ngx_http_v2_co
rst_stream:
+if (h2c->refused_streams++ > ngx_max(h2scf->concurrent_streams, 100)) {
+ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
+ "client sent too many refused streams");
+return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_NO_ERROR);
+}
+
if (ngx_http_v2_send_rst_stream(h2c, h2c->state.sid, status) != NGX_OK) {
return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_INTERNAL_ERROR);
}
diff --git a/src/http/v2/ngx_http_v2.h b/src/http/v2/ngx_http_v2.h
--- a/src/http/v2/ngx_http_v2.h
+++ b/src/http/v2/ngx_http_v2.h
@@ -131,6 +131,8 @@ struct ngx_http_v2_connection_s {
ngx_uint_t processing;
ngx_uint_t frames;
ngx_uint_t idle;
+ngx_uint_t new_streams;
+ngx_uint_t refused_streams;
ngx_uint_t priority_limit;
size_t send_window;
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: optimize the code for searching file in ngx_conf_open_file
Hello!
On Sat, Oct 07, 2023 at 06:09:18AM +0200, Davood Falahati wrote:
> # HG changeset patch
> # User Davood Falahati <0x0dav...@gmail.com>
> # Date 1696647746 -7200
> # Sat Oct 07 05:02:26 2023 +0200
> # Node ID ab14ea51bbb15331c9f44f14901d0fd378168647
> # Parent c073e545e1cdcc736f8869a012a78b2dd836eac9
> optimize the code for searching file in ngx_conf_open_file
>
> This patch combines two consecutive if statements into one and leveraging
> short circuits circuiting. In the current code, we check the lengths of
> file names and if they match, the file names are being compared.
>
> I see few other places in the code that writing multiple if statements are
> preferred over short circuit evaluation (e.g.
> http://hg.nginx.org/nginx/file/tip/src/http/ngx_http_file_cache.c#l1153). I
> wanted to make sure if it's a matter of community's taste or if it's in
> line with some performance considerations?
>
> diff -r c073e545e1cd -r ab14ea51bbb1 src/core/ngx_conf_file.c
> --- a/src/core/ngx_conf_file.c Thu May 18 23:42:22 2023 +0200
> +++ b/src/core/ngx_conf_file.c Sat Oct 07 05:02:26 2023 +0200
> @@ -927,11 +927,7 @@
> i = 0;
> }
>
> -if (full.len != file[i].name.len) {
> -continue;
> -}
> -
> -if (ngx_strcmp(full.data, file[i].name.data) == 0) {
> +if (full.len == file[i].name.len && ngx_strcmp(full.data,
> file[i].name.data) == 0) {
> return [i];
> }
> }
Thanks for your efforts, but we prefer the code as is.
There is no difference between these variants from the performance
point of view. On the other hand, the existing variant with
separate length comparison is certainly more readable. Further,
the suggested change uses incorrect indentation, which is just
wrong.
For more information about submitting patches, please see tips
here:
http://nginx.org/en/docs/contributing_changes.html
For more information about nginx coding style, please refer to the
code and here:
http://nginx.org/en/docs/dev/development_guide.html#code_style
Hope this helps.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Auth basic: Cache credentials if auth_basic_user_file is static
Hello! On Tue, Oct 03, 2023 at 03:46:05PM -0700, Toshihito Kikuchi wrote: > # HG changeset patch > # User Toshihito Kikuchi > # Date 1696359541 25200 > # Tue Oct 03 11:59:01 2023 -0700 > # Node ID e397ea6cfa85e85ae0865c5061397dc295fb7df1 > # Parent 3db945fda515014d220151046d02f3960bcfca0a > Auth basic: Cache credentials if auth_basic_user_file is static. > > In the current design, when auth_basic is on, every HTTP request triggers > file I/O (open, read, close) to the file specified in auth_basic_user_file. > Probably this is to allow auth_basic_user_file to contain variables. > > If the value is just a static text, however, there is no reason to read the > same file every request in every worker process. It unnecessarily consumes > system resources. > > With this patch, if auth_basic_user_file does not have any variables, we > cache its content in the location context at configuration time and use it > in all subsequent requests. If auth_basic_user_file contain variables, we > keep > the original behavior. As currently implemented, auth_basic_user_file is read at runtime, making it possible to change users and their passwords - which is a relatively common task - without reloading nginx itself. And this behaviour matches the one in Apache, which does the same. Changing this behaviour to read the password file while loading configuration (so any changes to the file won't be applied unless nginx is reloaded) would certainly break POLA, and needs some really good justification. Further, in typical setups the file is effectively cached by the OS itself, making the I/O operations mentioned almost free, especially compared to costs of typical password hash calculations. [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Updated OpenSSL and zlib versions
Hello! On Tue, Sep 26, 2023 at 02:14:18PM +0100, Yaroslav Zhuravlev wrote: > xml/en/docs/configure.xml| 6 +++--- > xml/en/docs/howto_build_on_win32.xml | 10 +- > xml/ru/docs/configure.xml| 6 +++--- > 3 files changed, 11 insertions(+), 11 deletions(-) > > > # HG changeset patch > # User Yaroslav Zhuravlev > # Date 1695733917 -3600 > # Tue Sep 26 14:11:57 2023 +0100 > # Node ID 32a78db44cf00ab54063905e2a2ff4e5092a7b28 > # Parent 1ad61bfc7630adf1d6460cf84cec484de4017326 > Updated OpenSSL and zlib versions. > > diff --git a/xml/en/docs/configure.xml b/xml/en/docs/configure.xml > --- a/xml/en/docs/configure.xml > +++ b/xml/en/docs/configure.xml > @@ -8,7 +8,7 @@ > link="/en/docs/configure.html" > lang="en" > - rev="22"> > + rev="23"> > > > > @@ -1270,7 +1270,7 @@ > > sets the path to the sources of the zlib library. > The library distribution (version > -1.1.31.2.11) needs to be downloaded from the > +1.1.31.2.13) needs to be downloaded from the > http://zlib.net;>zlib site and extracted. > The rest is done by nginx’s ./configure and > make. > @@ -1359,7 +1359,7 @@ > --pid-path=/usr/local/nginx/nginx.pid > --with-http_ssl_module > --with-pcre=../pcre2-10.39 > ---with-zlib=../zlib-1.2.11 > +--with-zlib=../zlib-1.2.13 > > > zlib 1.3 was recently released and works just fine, so this probably should be bumped to 1.3 instead. > diff --git a/xml/en/docs/howto_build_on_win32.xml > b/xml/en/docs/howto_build_on_win32.xml > --- a/xml/en/docs/howto_build_on_win32.xml > +++ b/xml/en/docs/howto_build_on_win32.xml > @@ -9,7 +9,7 @@ > link="/en/docs/howto_build_on_win32.html" > lang="en" > - rev="25"> > + rev="26"> > > > > @@ -81,8 +81,8 @@ > mkdir objs/lib > cd objs/lib > tar -xzf ../../pcre2-10.39.tar.gz > -tar -xzf ../../zlib-1.2.11.tar.gz > -tar -xzf ../../openssl-1.1.1m.tar.gz > +tar -xzf ../../zlib-1.2.13.tar.gz > +tar -xzf ../../openssl-3.0.10.tar.gz > > > > @@ -105,8 +105,8 @@ > --http-uwsgi-temp-path=temp/uwsgi_temp \ > --with-cc-opt=-DFD_SETSIZE=1024 \ > --with-pcre=objs/lib/pcre2-10.39 \ > ---with-zlib=objs/lib/zlib-1.2.11 \ > ---with-openssl=objs/lib/openssl-1.1.1m \ > +--with-zlib=objs/lib/zlib-1.2.13 \ > +--with-openssl=objs/lib/openssl-3.0.10 \ > --with-openssl-opt=no-asm \ > --with-http_ssl_module > And this used to match latest release of nginx/Windows, so zlib 1.2.13 and OpenSSL 3.0.10 are probably fine here. On the other hand, bumping to zlib 1.3 and OpenSSL 3.0.11 might be good enough as well (and will save an update to this file). > diff --git a/xml/ru/docs/configure.xml b/xml/ru/docs/configure.xml > --- a/xml/ru/docs/configure.xml > +++ b/xml/ru/docs/configure.xml > @@ -8,7 +8,7 @@ > link="/ru/docs/configure.html" > lang="ru" > - rev="22"> > + rev="23"> > > > > @@ -1250,7 +1250,7 @@ > > задаёт путь к исходным текстам библиотеки zlib. > Дистрибутив библиотеки (версию > -1.1.31.2.11) нужно взять на сайте > +1.1.31.2.13) нужно взять на сайте > http://zlib.net;>zlib и распаковать. > Всё остальное сделают ./configure nginx’а и > make. > @@ -1339,7 +1339,7 @@ > --pid-path=/usr/local/nginx/nginx.pid > --with-http_ssl_module > --with-pcre=../pcre2-10.39 > ---with-zlib=../zlib-1.2.11 > +--with-zlib=../zlib-1.2.13 > > > Otherwise looks good to me. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Improve performance when starting nginx with a lot of locations
_queue_sentinel(queue)) {
+ngx_queue_add(queue, tail);
+break;
+}
+
+if (q2 == ngx_queue_sentinel(tail)) {
+break;
+}
-prev = ngx_queue_prev(prev);
+if (cmp(q1, q2) <= 0) {
+q1 = ngx_queue_next(q1);
+continue;
+}
-} while (prev != ngx_queue_sentinel(queue));
+ngx_queue_remove(q2);
+ngx_queue_insert_before(q1, q2);
-ngx_queue_insert_after(prev, q);
+q2 = ngx_queue_head(tail);
}
}
diff --git a/src/core/ngx_queue.h b/src/core/ngx_queue.h
--- a/src/core/ngx_queue.h
+++ b/src/core/ngx_queue.h
@@ -47,6 +47,9 @@ struct ngx_queue_s {
(h)->prev = x
+#define ngx_queue_insert_before ngx_queue_insert_tail
+
+
#define ngx_queue_head(h) \
(h)->next
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 2 of 2] Added "max_shutdown_workers" directive
es[i].exiting,
> ngx_processes[i].exited,
> ngx_processes[i].detached,
> ngx_processes[i].respawn,
> - ngx_processes[i].just_spawn);
> + ngx_processes[i].just_spawn,
> + ngx_processes[i].old);
>
> if (ngx_processes[i].detached || ngx_processes[i].pid == -1) {
> return;
> @@ -563,15 +630,16 @@ ngx_reap_children(ngx_cycle_t *cycle)
> live = 0;
> for (i = 0; i < ngx_last_process; i++) {
>
> -ngx_log_debug7(NGX_LOG_DEBUG_EVENT, cycle->log, 0,
> - "child: %i %P e:%d t:%d d:%d r:%d j:%d",
> +ngx_log_debug8(NGX_LOG_DEBUG_EVENT, cycle->log, 0,
> + "child: %i %P e:%d t:%d d:%d r:%d j:%d o:%d",
> i,
> ngx_processes[i].pid,
> ngx_processes[i].exiting,
> ngx_processes[i].exited,
> ngx_processes[i].detached,
> ngx_processes[i].respawn,
> - ngx_processes[i].just_spawn);
> + ngx_processes[i].just_spawn,
> + ngx_processes[i].old);
>
> if (ngx_processes[i].pid == -1) {
> continue;
> @@ -660,6 +728,16 @@ ngx_reap_children(ngx_cycle_t *cycle)
> ngx_processes[i].pid = -1;
> }
>
Note: when a process dies, it is restarted if the "respawn" flag
is set, regardless of the "old" flag. The "old" flag is preserved
even if it is no longer relevant after the respawn: the process is
restarted with the new configuration, and there is no need to
restart it again.
> +if (ngx_processes[i].old
> +&& ngx_processes[i].exiting
> +&& !ngx_terminate
> +&& !ngx_quit)
> +{
> +if (ngx_restart_worker_process(cycle)) {
> +live = 1;
> +}
> +}
> +
> } else if (ngx_processes[i].exiting || !ngx_processes[i].detached) {
> live = 1;
> }
> @@ -669,6 +747,53 @@ ngx_reap_children(ngx_cycle_t *cycle)
> }
>
>
> +static ngx_uint_t
> +ngx_restart_worker_process(ngx_cycle_t *cycle)
> +{
> +ngx_int_t i, j, m;
> +ngx_core_conf_t *ccf;
> +
> +ccf = (ngx_core_conf_t *) ngx_get_conf(cycle->conf_ctx, ngx_core_module);
> +
> +j = -1;
> +m = ccf->max_shutdown_workers;
> +
> +if (m == 0) {
> +return 0;
> +}
> +
> +for (i = 0; i < ngx_last_process; i++) {
> +
> +if (ngx_processes[i].pid == -1 || !ngx_processes[i].old) {
> +continue;
> +}
> +
> +if (!ngx_processes[i].exiting && !ngx_processes[i].exited) {
The "exited" check here is not needed to catch the processes we
were called for, or other processes being shut down - they already
have "exiting" flag set.
On the other hand, this can catch unexpectedly died processes
(the ones which are still waiting for restart). As per the code,
such processes will be counted against max_shutdown_workers as if
they are shutting down - this looks wrong and can cause reload
hang.
> +j = i;
> +continue;
> +}
> +
> +if (--m == 0) {
> +return 0;
> +}
> +}
> +
> +if (j == -1) {
> +return 0;
> +}
> +
> +ngx_start_worker_process(cycle, (intptr_t) ngx_processes[j].data,
> + NGX_PROCESS_RESPAWN);
> +
> +ngx_msleep(100);
> +
> +ngx_signal_worker_process(cycle, j,
> + ngx_signal_value(NGX_SHUTDOWN_SIGNAL));
Also note that the "exited" check above implies that there can be
multiple exited processes at the same time, but we only restart
just one process here.
It might be a good idea to restart multiple processes at the same
time: sleeping for 100ms for each process means that if, for
example, 64 processes will exit at the same time after shutdown
timeout expiration, master process will be unresponsive for 6
seconds.
Also, this looks very close to ngx_restart_worker_processes(). It
might be a good idea to rework things to ensure unified approach
to restarting processes instead of providing multiple duplicate
ways.
> +
> +return 1;
> +}
> +
> +
> static void
> ngx_master_process_exit(ngx_cycle_t *cycle)
> {
Overall, I tend to think that the suggested code is very fragile,
and I would rather consider re-thinking the approach.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Linux packages: removed Ubuntu 22.10 'kinetic' due to EOL
Hello! On Fri, Sep 22, 2023 at 03:12:23PM -0700, Konstantin Pavlov wrote: > # HG changeset patch > # User Konstantin Pavlov > # Date 1695420683 25200 > # Fri Sep 22 15:11:23 2023 -0700 > # Node ID 1ad61bfc7630adf1d6460cf84cec484de4017326 > # Parent ac4191d05fdf12dbc977a3a26dfde2799d301283 > Linux packages: removed Ubuntu 22.10 'kinetic' due to EOL. > > diff -r ac4191d05fdf -r 1ad61bfc7630 xml/en/linux_packages.xml > --- a/xml/en/linux_packages.xml Thu Sep 14 21:20:14 2023 +0100 > +++ b/xml/en/linux_packages.xml Fri Sep 22 15:11:23 2023 -0700 > @@ -7,7 +7,7 @@ > link="/en/linux_packages.html" > lang="en" > - rev="89"> > + rev="90"> > > > > @@ -88,11 +88,6 @@ versions: > > > > -22.10 “kinetic” > -x86_64, aarch64/arm64 > - > - > - > 23.04 “lunar” > x86_64, aarch64/arm64 > > diff -r ac4191d05fdf -r 1ad61bfc7630 xml/ru/linux_packages.xml > --- a/xml/ru/linux_packages.xml Thu Sep 14 21:20:14 2023 +0100 > +++ b/xml/ru/linux_packages.xml Fri Sep 22 15:11:23 2023 -0700 > @@ -7,7 +7,7 @@ > link="/ru/linux_packages.html" > lang="ru" > - rev="89"> > + rev="90"> > > > > @@ -88,11 +88,6 @@ > > > > -22.10 “kinetic” > -x86_64, aarch64/arm64 > - > - > - > 23.04 “lunar” > x86_64, aarch64/arm64 > Looks fine. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Mail: allow auth to the proxy without auth to the backend
Hello! On Tue, Sep 19, 2023 at 12:28:49PM +0200, Arnout Engelen wrote: > # HG changeset patch > # User Arnout Engelen > # Date 1695027670 -7200 > # Mon Sep 18 11:01:10 2023 +0200 > # Node ID 9606e589b9537495c0457383048ac6888be0e7b4 > # Parent daf8f5ba23d8e9955b22782d945f9c065f4b6baa > Mail: allow auth to the proxy without auth to the backend > > Currently, when the client authenticates itself to the nginx > mail proxy, the mail proxy also authenticates itself to the > backend. > > I encountered a situation where I wanted the proxy to require > authentication, and forward the mail to a (local/firewalled) > mailserver that does not have authentication configured. I > created the patch below to support that. > > I'm providing this patch primarily for feedback at this point: > while it does work for my scenario and pass the nginx-tests, > it likely needs additional cleanup and testing. I'd like your > thoughs on whether this change makes sense in the first place, > and whether this is generally a reasonable approach - if so I'll > clean up the patch further. > > My approach is to allow the authentication server to return a > 'Auth-Method: none' header, in which case the proxy will not > attempt to authenticate to the backend but instead wait for > the 'MAIL FROM' from the client. > > You'll notice I've added a 'proxy_auth_method'. The reason I didn't > overwrite 'auth_method' is that 'auth_method' is also used to determine > whether to confirm the authentication to the client. Is that acceptable > from a binary compatibility perspective? > > Looking forward to hearing your thoughts! From the description it is not clear why "proxy_smtp_auth off;" (which is the default and implies that nginx won't try to authenticate against SMTP backends) does not work for you. Could you please elaborate? [...] -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
