[PATCH] Upstream: prioritise Cache-Control over Expires

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1649889268 -10800
#  Thu Apr 14 01:34:28 2022 +0300
# Node ID ed7a2c031475bcb252952a467c184c94652b926a
# Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Upstream: prioritise Cache-Control over Expires.

RFC7234 explicitly says that cache recipient MUST ignore Expires
header if response includes Cache-Control header with max-age or
s-maxage directives. Previously Cache-Control was ignored if it
was after Expires in reply headers.

At the same time this patch makes more stable behaviour of using
latest value of header Cache-Control even if previous value was 0.
Ticket #964 for more information.
---
 src/http/ngx_http_upstream.c | 25 +
 src/http/ngx_http_upstream.h | 15 +++
 2 files changed, 40 insertions(+)

diff -r a736a7a613ea -r ed7a2c031475 src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c  Tue Feb 08 17:35:27 2022 +0300
+++ b/src/http/ngx_http_upstream.c  Thu Apr 14 01:34:28 2022 +0300
@@ -868,6 +868,7 @@
 }
 
 u->cacheable = 1;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_DEFAULT;
 
 c = r->cache;
 
@@ -970,6 +971,7 @@
 case NGX_HTTP_CACHE_SCARCE:
 
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_SCARCE;
 
 break;
 
@@ -992,6 +994,7 @@
 
 if (ngx_http_upstream_cache_check_range(r, u) == NGX_DECLINED) {
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_RANGE;
 }
 
 r->cached = 0;
@@ -3105,6 +3108,7 @@
 
 case NGX_DECLINED:
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_NO_CACHE;
 break;
 
 default: /* NGX_OK */
@@ -3165,6 +3169,7 @@
 
 } else {
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_INVALID;
 }
 }
 
@@ -4690,6 +4695,7 @@
 #if (NGX_HTTP_CACHE)
 if (!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_SET_COOKIE)) {
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_COOKIE;
 }
 #endif
 
@@ -4747,6 +4753,7 @@
 || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
 {
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_NO_CACHE;
 return NGX_OK;
 }
 
@@ -4772,15 +4779,21 @@
 }
 
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_CACHECTRL0;
 return NGX_OK;
 }
 
 if (n == 0) {
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_CACHECTRL0;
 return NGX_OK;
 }
 
 r->cache->valid_sec = ngx_time() + n;
+u->cacheable_reason &= 
~(NGX_HTTP_CACHEABLE_CACHECTRL0|NGX_HTTP_CACHEABLE_EXPIRES);
+if (u->cacheable_reason == NGX_HTTP_CACHEABLE_DEFAULT) {
+u->cacheable = 1;
+}
 }
 
 p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
@@ -4800,9 +4813,13 @@
 }
 
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_CACHECTRL1;
 return NGX_OK;
 }
 
+if (u->cacheable_reason == NGX_HTTP_CACHEABLE_DEFAULT) {
+u->cacheable = 1;
+}
 r->cache->updating_sec = n;
 r->cache->error_sec = n;
 }
@@ -4823,10 +4840,15 @@
 }
 
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_CACHECTRL2;
 return NGX_OK;
 }
 
 r->cache->error_sec = n;
+u->cacheable_reason &= ~NGX_HTTP_CACHEABLE_EXPIRES;
+if (u->cacheable_reason == NGX_HTTP_CACHEABLE_DEFAULT) {
+u->cacheable = 1;
+}
 }
 }
 #endif
@@ -4864,6 +4886,7 @@
 
 if (expires == NGX_ERROR || expires < ngx_time()) {
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_EXPIRES;
 return NGX_OK;
 }
 
@@ -4907,6 +4930,7 @@
 switch (n) {
 case 0:
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_XACCEL;
 /* fall through */
 
 case NGX_ERROR:
@@ -5067,6 +5091,7 @@
 || (h->value.len == 1 && h->value.data[0] == '*'))
 {
 u->cacheable = 0;
+u->cacheable_reason |= NGX_HTTP_CACHEABLE_VARY;
 }
 
 r->cache->vary = h->value;
diff -r a736a7a613ea -r ed7a2c031475 src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h  Tue Feb 08 17:35:27 2022 +0300
+++ b/src/http/ngx_http_upstream.h  Thu Apr 14 01:34:28 2022 +0300
@@ -320,6 +320,20 @@
 ngx_http_upstream_t *u);
 
 
+#define NGX_HTTP_CACHEABLE_DEFAULT  0x8000
+#define NGX_HTTP_CACHEABLE_SCARCE   0x0001
+#define NGX_HTTP_CACHEABLE_RANGE0x0002
+#define NGX_HTTP_CACHEABLE_INVALID  0x0004
+#define NGX_HTTP_CACHEABLE_NO_CACHE 0x0010
+#define NGX_HTTP_CACHEABLE_EXPIRES  0x0020
+#define NGX_HTTP_CACHEABLE_

[PATCH] Tests: added Expires and Cache-Control headers test

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1649976970 -10800
#  Fri Apr 15 01:56:10 2022 +0300
# Node ID 39dea3973d47e0bcd226beb3c6554dcdc0e26495
# Parent  0c50a00e67334659d58d3cf7cb81fcf5872a8285
Tests: added Expires and Cache-Control headers test

diff -r 0c50a00e6733 -r 39dea3973d47 proxy_cache_expires_cache_control.t
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/proxy_cache_expires_cache_control.t   Fri Apr 15 01:56:10 2022 +0300
@@ -0,0 +1,88 @@
+#!/usr/bin/perl
+
+# (C) Georgii Dzebisashvili 
+
+# Tests for cache management regarding https://trac.nginx.org/nginx/ticket/964
+
+###
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+
+use File::Find;
+
+###
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+my $scriptDir = $FindBin::Bin;
+my $cacheDir = $scriptDir."/testcache";
+my $tempDir = $scriptDir."/testtemp";
+
+my $t = Test::Nginx->new()->plan(5)->write_file_expand('nginx.conf', <<"EOF");
+
+error_log stderr info;
+daemon off;
+events {
+worker_connections 64;
+}
+http {
+access_log off;
+proxy_cache_path $cacheDir levels=2 keys_zone=cache_zone:1m inactive=24h 
max_size=10m;
+proxy_temp_path $tempDir;
+server {
+listen 127.0.0.1:8080;
+location / {
+proxy_pass http://127.0.0.2:8080;
+proxy_cache cache_zone;
+}
+}
+server {
+listen 127.0.0.2:8080;
+location /success {
+add_header Cache-Control "max-age=3600";
+add_header Expires "Tue, 15 Nov 1994 12:45:26 GMT";
+return 200 "Hello world!";
+}
+location /fail {
+add_header Expires "Tue, 15 Nov 1994 12:45:26 GMT";
+add_header Cache-Control "max-age=3600";
+return 200 "Hello world!";
+}
+}
+}
+
+
+EOF
+
+$t->run();
+
+###
+
+my $counter = 0;
+
+sub fileMatchCallback {
+-f && $counter++; # Only count files
+}
+
+my $r;
+
+$r = http_get('/success');
+like($r, qr/Cache-Control/, 'cache-control-first-cache-control-is-present');
+like($r, qr/Expires/, 'cache-control-first-expires-is-present');
+
+$r = http_get('/fail');
+like($r, qr/Cache-Control/, 'expires-first-cache-control-is-present');
+like($r, qr/Expires/, 'expires-first-expires-is-present');
+
+find(\&fileMatchCallback, $cacheDir);
+
+is($counter, 2, 'overall number of cached requests');
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

[Vadim Fedorenko via nginx-devel]

Hi Maxim!

On 17.04.2022 02:55, Maxim Dounin wrote:

Hello!


[...]

Thanks for the patch.

I'm quite sceptical about attempts to fix this by introducing
various flags and reverting cacheable status back to 1.  This is
not how it should be fixed.


Yeah, the solution might be a bit complicated, but I couldn't find
another way without breaking concept of independent header parsing.
Could you please suggest something if you think that current approach
is wrong? The ticket that this patch tries to fix is 6 years old and
still has discussions going on without any solution.

Thanks,
Vadim
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

[Vadim Fedorenko via nginx-devel]

Hi Yugo!

Looks like another solution with the same concept - store additional information
about several headers. The second patch you proposed looks a bit overcomplicated
inside this additional function. I don't see any reasons to store information
about "Vary" header. AFAIU, weed special information about 3 headers: "Expires",
"Cache-Control" and "X-Accel-Expires". And we can call 
"ngx_http_upstream_cache_validate_regardless_order" only when cacheable is still

enabled after parsing all other headers. That approach will simplify the logic
inside ngx_http_upstream_cache_validate_regardless_order. I will prepare an
improved version if you don't mind.

Thanks,
Vadim

On 18.04.2022 02:14, Yugo Horie wrote:
We're also interested in this issue because the CDN provider like us is 
concerned about the problems with the headers that determine the cache freshness.
It is often a serious problem used in Nginx as the origin server or the 
intermediate cache server that has such problems.

In those days, we also proposed a patch to resolve it.
https://mailman.nginx.org/archives/list/nginx-devel@nginx.org/thread/ZQVPGEXAAHVL23TBBNMXLP4KDGDJCVTW/ 
<https://mailman.nginx.org/archives/list/nginx-devel@nginx.org/thread/ZQVPGEXAAHVL23TBBNMXLP4KDGDJCVTW/>


But it seems to have declined. We sincerely wish to resolve it and intend to do 
anything.


Thanks,
Yugo Horie

2022年4月18日(月) 4:51 Vadim Fedorenko via nginx-devel <mailto:nginx-devel@nginx.org>>:


Hi Maxim!

On 17.04.2022 02:55, Maxim Dounin wrote:
 > Hello!
 >
 >
 > [...]
 >
 > Thanks for the patch.
 >
 > I'm quite sceptical about attempts to fix this by introducing
 > various flags and reverting cacheable status back to 1.  This is
 > not how it should be fixed.
 >
Yeah, the solution might be a bit complicated, but I couldn't find
another way without breaking concept of independent header parsing.
Could you please suggest something if you think that current approach
is wrong? The ticket that this patch tries to fix is 6 years old and
still has discussions going on without any solution.

Thanks,
Vadim
___
nginx-devel mailing list -- nginx-devel@nginx.org 
<mailto:nginx-devel@nginx.org>
To unsubscribe send an email to nginx-devel-le...@nginx.org
<mailto:nginx-devel-le...@nginx.org>


___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org

[PATCH v2] Upstream: prioritise X-Accel-Expire over Cache-Control and Expire

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1650406016 -10800
#  Wed Apr 20 01:06:56 2022 +0300
# Node ID a75449f4b5c98df0e5a0041eeaa1be5c82d92fea
# Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Upstream: prioritise X-Accel-Expire over Cache-Control and Expire

RFC7234 explicitly says that cache recipient MUST ignore Expires
header if response includes Cache-Control header with max-age or
s-maxage directives. Previously Cache-Control was ignored if it
was after Expires in reply headers.

At the same time documentation states that there is special header
X-Accel-Expires that controls the behavior of nginx and should be
prioritized over other cache-specific headers and this patch
implements this priority.

More informantion could be found in ticket #964.
---
 src/http/ngx_http_upstream.c | 62 +---
 src/http/ngx_http_upstream.h |  6 
 2 files changed, 50 insertions(+), 18 deletions(-)

diff -r a736a7a613ea -r a75449f4b5c9 src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c  Tue Feb 08 17:35:27 2022 +0300
+++ b/src/http/ngx_http_upstream.c  Wed Apr 20 01:06:56 2022 +0300
@@ -2350,6 +2350,32 @@
 
 
 static void
+ngx_http_upstream_validate_cache_headers(ngx_http_request_t *r, 
ngx_http_upstream_t *u)
+{
+ngx_http_upstream_headers_in_t *uh = &u->headers_in;
+if (uh->x_accel_expires != NULL  &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_EXPIRES)) {
+u->cacheable = uh->x_accel_expires_c;
+r->cache->valid_sec = uh->x_accel_expires_n;
+return;
+}
+
+if (uh->cache_control.elts != NULL &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_CACHE_CONTROL)) {
+u->cacheable = uh->cache_control_c;
+r->cache->valid_sec = uh->cache_control_n;
+return;
+}
+
+if (uh->expires != NULL  &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_EXPIRES)) {
+u->cacheable = uh->expires_c;
+r->cache->valid_sec = uh->expires_n;
+}
+}
+
+
+static void
 ngx_http_upstream_process_header(ngx_http_request_t *r, ngx_http_upstream_t *u)
 {
 ssize_tn;
@@ -2468,6 +2494,11 @@
 
 continue;
 }
+#if (NGX_HTTP_CACHE)
+if (u->cacheable) {
+ngx_http_upstream_validate_cache_headers(r, u);
+}
+#endif
 
 break;
 }
@@ -4735,10 +4766,6 @@
 return NGX_OK;
 }
 
-if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
-return NGX_OK;
-}
-
 start = h->value.data;
 last = start + h->value.len;
 
@@ -4746,7 +4773,7 @@
 || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL
 || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
 {
-u->cacheable = 0;
+u->headers_in.cache_control_c = 0;
 return NGX_OK;
 }
 
@@ -4771,7 +4798,6 @@
 continue;
 }
 
-u->cacheable = 0;
 return NGX_OK;
 }
 
@@ -4780,7 +4806,8 @@
 return NGX_OK;
 }
 
-r->cache->valid_sec = ngx_time() + n;
+u->headers_in.cache_control_c = 1;
+u->headers_in.cache_control_n = ngx_time() + n;
 }
 
 p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
@@ -4856,18 +4883,18 @@
 return NGX_OK;
 }
 
-if (r->cache->valid_sec != 0) {
+expires = ngx_parse_http_time(h->value.data, h->value.len);
+
+if (expires == NGX_ERROR || expires < ngx_time()) {
 return NGX_OK;
 }
 
-expires = ngx_parse_http_time(h->value.data, h->value.len);
-
-if (expires == NGX_ERROR || expires < ngx_time()) {
-u->cacheable = 0;
-return NGX_OK;
-}
-
-r->cache->valid_sec = expires;
+if (u->headers_in.expires_c) {
+expires = ngx_min(expires, u->headers_in.expires_n);
+}
+u->headers_in.expires_n = expires;
+u->headers_in.expires_c = 1;
+
 }
 #endif
 
@@ -4906,14 +4933,12 @@
 
 switch (n) {
 case 0:
-u->cacheable = 0;
-/* fall through */
-
 case NGX_ERROR:
 return NGX_OK;
 
 default:
-r->cache->valid_sec = ngx_time() + n;
+u->headers_in.x_accel_expires_c = 1;
+u->headers_in.x_accel_expires_n = ngx_time() + n;
 return NGX_OK;
 }
 }
@@ -4924,7 +4949,8 @@
 n = ngx_atoi(p, len);
 
 if (n != NGX_ERROR) {
-r->cache->valid_sec = n;
+u->headers_in.x_accel_expires_c = 1;
+u->headers_in.x_accel_expires_n = ngx_time() + n;
 }
 }
 #endif
diff -r a736a7a613ea -r a75449f4b5c9 src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h  Tue Feb 08 17:35:27 2022 +0300
+++ b/src/http/ngx_http_upstream.h  Wed Apr 20 01:06:56 2022 +0300
@@ -294,9 +294,15 @@
 
 off_tcontent_length_n;
 time_t   last_modified

[PATCH v2] Tests: added Expires and Cache-Control headers test

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1649976970 -10800
#  Fri Apr 15 01:56:10 2022 +0300
# Node ID 3d5684530a8ef228cd7f20ff3e51f9ea5e77f2ec
# Parent  0c50a00e67334659d58d3cf7cb81fcf5872a8285
Tests: added Expires and Cache-Control headers test

diff -r 0c50a00e6733 -r 3d5684530a8e proxy_cache_expires_cache_control.t
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/proxy_cache_expires_cache_control.t   Fri Apr 15 01:56:10 2022 +0300
@@ -0,0 +1,99 @@
+#!/usr/bin/perl
+
+# (C) Georgii Dzebisashvili 
+# (C) Vadim Fedorenko 
+
+# Tests for cache management regarding https://trac.nginx.org/nginx/ticket/964
+
+###
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+
+use File::Find;
+
+###
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+my $scriptDir = $FindBin::Bin;
+my $cacheDir = $scriptDir."/testcache";
+my $tempDir = $scriptDir."/testtemp";
+
+my $t = Test::Nginx->new()->plan(8)->write_file_expand('nginx.conf', <<"EOF");
+
+daemon off;
+events {
+worker_connections 64;
+}
+http {
+access_log off;
+proxy_cache_path $cacheDir levels=2 keys_zone=cache_zone:1m inactive=24h 
max_size=10m;
+proxy_temp_path $tempDir;
+server {
+listen 127.0.0.1:8080;
+location / {
+proxy_pass http://127.0.0.2:8080;
+proxy_cache cache_zone;
+}
+}
+server {
+listen 127.0.0.2:8080;
+location /success {
+add_header Cache-Control "max-age=3600";
+add_header Expires "Tue, 15 Nov 1994 12:45:26 GMT";
+return 200 "Hello world!";
+}
+location /fail {
+add_header Expires "Tue, 15 Nov 1994 12:45:26 GMT";
+add_header Cache-Control "max-age=3600";
+return 200 "Hello world!";
+}
+location /store_xaccelexpire {
+add_header Expires "Tue, 15 Nov 1994 12:45:26 GMT";
+add_header Cache-Control "no-cache";
+add_header X-Accel-Expires "60";
+return 200 "Hello world!";
+}
+}
+}
+
+
+EOF
+
+$t->run();
+
+###
+
+my $counter = 0;
+
+sub fileMatchCallback {
+-f && $counter++; # Only count files
+}
+
+my $r;
+
+$r = http_get('/success');
+like($r, qr/Cache-Control/, 'cache-control-first-cache-control-is-present');
+like($r, qr/Expires/, 'cache-control-first-expires-is-present');
+
+$r = http_get('/fail');
+like($r, qr/Cache-Control/, 'expires-first-cache-control-is-present');
+like($r, qr/Expires/, 'expires-first-expires-is-present');
+
+$r = http_get('/store_xaccelexpire');
+like($r, qr/Cache-Control/, 'cache-control-with-xaccel-is-present');
+like($r, qr/Expires/, 'expires-with-xaccel-is-present');
+unlike($r, qr/X-Accel-Expires/, 'xaccel-is-not-present');
+
+find(\&fileMatchCallback, $cacheDir);
+
+is($counter, 3, 'overall number of cached requests');
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org

Re: [PATCH v2] Upstream: prioritise X-Accel-Expire over Cache-Control and Expire

[Vadim Fedorenko via nginx-devel]

Hi Yugo,

Looks like you are right, I will post v3 with updates soon.

Thanks,
Vadim

On 20.04.2022 05:22, Yugo Horie wrote:

Hi, Vadim
We're pleased to have you share your patch!

src/http/ngx_http_upstream.c
```
4803 if (n == 0) {
4804 u->cacheable = 0;
4805 return NGX_OK;
4806 }
```
We consider that X-Accel-Expires seems not to prioritize
Cache-Control:max-age=0, when it parses after Cache-Control:max-age=0.
Because u->cacheable has been 0 according to this code it cannot apply
the X-Accel-Expires values with
`ngx_http_upstream_validate_cache_headers`.
Could we set u->headers_in.cache_control_c to 0 or only return NGX_OK
to do nothing here instead of u->cacheable set 0?

Best,

Yugo Horie


2022年4月20日(水) 7:20 Vadim Fedorenko via nginx-devel :


# HG changeset patch
# User Vadim Fedorenko 
# Date 1650406016 -10800
#  Wed Apr 20 01:06:56 2022 +0300
# Node ID a75449f4b5c98df0e5a0041eeaa1be5c82d92fea
# Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Upstream: prioritise X-Accel-Expire over Cache-Control and Expire

RFC7234 explicitly says that cache recipient MUST ignore Expires
header if response includes Cache-Control header with max-age or
s-maxage directives. Previously Cache-Control was ignored if it
was after Expires in reply headers.

At the same time documentation states that there is special header
X-Accel-Expires that controls the behavior of nginx and should be
prioritized over other cache-specific headers and this patch
implements this priority.

More informantion could be found in ticket #964.
---
  src/http/ngx_http_upstream.c | 62 +---
  src/http/ngx_http_upstream.h |  6 
  2 files changed, 50 insertions(+), 18 deletions(-)

diff -r a736a7a613ea -r a75449f4b5c9 src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c  Tue Feb 08 17:35:27 2022 +0300
+++ b/src/http/ngx_http_upstream.c  Wed Apr 20 01:06:56 2022 +0300
@@ -2350,6 +2350,32 @@


  static void
+ngx_http_upstream_validate_cache_headers(ngx_http_request_t *r, 
ngx_http_upstream_t *u)
+{
+ngx_http_upstream_headers_in_t *uh = &u->headers_in;
+if (uh->x_accel_expires != NULL  &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_EXPIRES)) {
+u->cacheable = uh->x_accel_expires_c;
+r->cache->valid_sec = uh->x_accel_expires_n;
+return;
+}
+
+if (uh->cache_control.elts != NULL &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_CACHE_CONTROL)) {
+u->cacheable = uh->cache_control_c;
+r->cache->valid_sec = uh->cache_control_n;
+return;
+}
+
+if (uh->expires != NULL  &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_EXPIRES)) {
+u->cacheable = uh->expires_c;
+r->cache->valid_sec = uh->expires_n;
+}
+}
+
+
+static void
  ngx_http_upstream_process_header(ngx_http_request_t *r, ngx_http_upstream_t 
*u)
  {
  ssize_tn;
@@ -2468,6 +2494,11 @@

  continue;
  }
+#if (NGX_HTTP_CACHE)
+if (u->cacheable) {
+ngx_http_upstream_validate_cache_headers(r, u);
+}
+#endif

  break;
  }
@@ -4735,10 +4766,6 @@
  return NGX_OK;
  }

-if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
-return NGX_OK;
-}
-
  start = h->value.data;
  last = start + h->value.len;

@@ -4746,7 +4773,7 @@
  || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL
  || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
  {
-u->cacheable = 0;
+u->headers_in.cache_control_c = 0;
  return NGX_OK;
  }

@@ -4771,7 +4798,6 @@
  continue;
  }

-u->cacheable = 0;
  return NGX_OK;
  }

@@ -4780,7 +4806,8 @@
  return NGX_OK;
  }

-r->cache->valid_sec = ngx_time() + n;
+u->headers_in.cache_control_c = 1;
+u->headers_in.cache_control_n = ngx_time() + n;
  }

  p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
@@ -4856,18 +4883,18 @@
  return NGX_OK;
  }

-if (r->cache->valid_sec != 0) {
+expires = ngx_parse_http_time(h->value.data, h->value.len);
+
+if (expires == NGX_ERROR || expires < ngx_time()) {
  return NGX_OK;
  }

-expires = ngx_parse_http_time(h->value.data, h->value.len);
-
-if (expires == NGX_ERROR || expires < ngx_time()) {
-u->cacheable = 0;
-return NGX_OK;
-}
-
-r->cache->valid_sec = expires;
+if (u->headers_in.expires_c) {
+expires = ngx_min(expires, u->headers_in.expires_n);
+}
+u->headers_in.e

[PATCH v3] Upstream: prioritise X-Accel-Expire over Cache-Control and Expire

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1650406016 -10800
#  Wed Apr 20 01:06:56 2022 +0300
# Node ID e04dac22328020cf8d8abcc4863b982b513b0c80
# Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Upstream: prioritise X-Accel-Expire over Cache-Control and Expire

RFC7234 explicitly says that cache recipient MUST ignore Expires
header if response includes Cache-Control header with max-age or
s-maxage directives. Previously Cache-Control was ignored if it
was after Expires in reply headers.

At the same time documentation states that there is special header
X-Accel-Expires that controls the behavior of nginx and should be
prioritized over other cache-specific headers and this patch
implements this priority.

More informantion could be found in ticket #964.
---
 src/http/ngx_http_upstream.c | 62 +---
 src/http/ngx_http_upstream.h |  6 
 2 files changed, 50 insertions(+), 18 deletions(-)

diff -r a736a7a613ea -r e04dac223280 src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c  Tue Feb 08 17:35:27 2022 +0300
+++ b/src/http/ngx_http_upstream.c  Wed Apr 20 01:06:56 2022 +0300
@@ -2350,6 +2350,32 @@
 
 
 static void
+ngx_http_upstream_validate_cache_headers(ngx_http_request_t *r, 
ngx_http_upstream_t *u)
+{
+ngx_http_upstream_headers_in_t *uh = &u->headers_in;
+if (uh->x_accel_expires != NULL  &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_EXPIRES)) {
+u->cacheable = uh->x_accel_expires_c;
+r->cache->valid_sec = uh->x_accel_expires_n;
+return;
+}
+
+if (uh->cache_control.elts != NULL &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_CACHE_CONTROL)) {
+u->cacheable = uh->cache_control_c;
+r->cache->valid_sec = uh->cache_control_n;
+return;
+}
+
+if (uh->expires != NULL  &&
+!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_EXPIRES)) {
+u->cacheable = uh->expires_c;
+r->cache->valid_sec = uh->expires_n;
+}
+}
+
+
+static void
 ngx_http_upstream_process_header(ngx_http_request_t *r, ngx_http_upstream_t *u)
 {
 ssize_tn;
@@ -2468,6 +2494,11 @@
 
 continue;
 }
+#if (NGX_HTTP_CACHE)
+if (u->cacheable) {
+ngx_http_upstream_validate_cache_headers(r, u);
+}
+#endif
 
 break;
 }
@@ -4735,10 +4766,6 @@
 return NGX_OK;
 }
 
-if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
-return NGX_OK;
-}
-
 start = h->value.data;
 last = start + h->value.len;
 
@@ -4746,7 +4773,7 @@
 || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL
 || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
 {
-u->cacheable = 0;
+u->headers_in.cache_control_c = 0;
 return NGX_OK;
 }
 
@@ -4771,16 +4798,15 @@
 continue;
 }
 
-u->cacheable = 0;
 return NGX_OK;
 }
 
 if (n == 0) {
-u->cacheable = 0;
 return NGX_OK;
 }
 
-r->cache->valid_sec = ngx_time() + n;
+u->headers_in.cache_control_c = 1;
+u->headers_in.cache_control_n = ngx_time() + n;
 }
 
 p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
@@ -4856,18 +4882,18 @@
 return NGX_OK;
 }
 
-if (r->cache->valid_sec != 0) {
+expires = ngx_parse_http_time(h->value.data, h->value.len);
+
+if (expires == NGX_ERROR || expires < ngx_time()) {
 return NGX_OK;
 }
 
-expires = ngx_parse_http_time(h->value.data, h->value.len);
-
-if (expires == NGX_ERROR || expires < ngx_time()) {
-u->cacheable = 0;
-return NGX_OK;
-}
-
-r->cache->valid_sec = expires;
+if (u->headers_in.expires_c) {
+expires = ngx_min(expires, u->headers_in.expires_n);
+}
+u->headers_in.expires_n = expires;
+u->headers_in.expires_c = 1;
+
 }
 #endif
 
@@ -4906,14 +4932,12 @@
 
 switch (n) {
 case 0:
-u->cacheable = 0;
-/* fall through */
-
 case NGX_ERROR:
 return NGX_OK;
 
 default:
-r->cache->valid_sec = ngx_time() + n;
+u->headers_in.x_accel_expires_c = 1;
+u->headers_in.x_accel_expires_n = ngx_time() + n;
 return NGX_OK;
 }
 }
@@ -4924,7 +4948,8 @@
 n = ngx_atoi(p, len);
 
 if (n != NGX_ERROR) {
-r->cache->valid_sec = n;
+u->headers_in.x_accel_expires_c = 1;
+u->headers_in.x_accel_expires_n = ngx_time() + n;
 }
 }
 #endif
diff -r a736a7a613ea -r e04dac223280 src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h  Tue Feb 08 17:35:27 2022 +0300
+++ b/src/http/ngx_http_upstream.h  Wed Apr 20 01:06:56 2022 +0300
@@ -294,9 +294,15 @@
 
 off_tcontent_length_n;
 time_t  

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

[Vadim Fedorenko via nginx-devel]

Hi Maxim!

Thanks for the feedback. As you might have already seen, I sent another version
of this patch which addresses some of style and readability issues as well as
X-Accel-Expires issue in v3.

Regarding RFC issue. RFC 7234 Section 5.1 explicitly states that Expires MUST be
ignored if reponse contains Cache-Control header with max-age or s-maxage
directives. That's why I said that Nginx server configured to cache responses
and to rely on headers to determine the freshness violates RFC. And this is true
for a subset of absolutely legitimate responses, and it was done intended to
actually optimize the internal processing but not because of configuration
optimizations.

Hope you will have some time to review version 3 of my patch even though it
might not clearly apply on top of the patchset you sent couple of days ago.

Thanks,
Vadim

On 22.04.2022 19:23, Maxim Dounin wrote:

Hello!

On Tue, Apr 19, 2022 at 10:59:12PM +0100, Vadim Fedorenko wrote:


On 19.04.2022 16:01, Maxim Dounin wrote:


On Sun, Apr 17, 2022 at 08:50:25PM +0100, Vadim Fedorenko via nginx-devel wrote:


On 17.04.2022 02:55, Maxim Dounin wrote:


I'm quite sceptical about attempts to fix this by introducing
various flags and reverting cacheable status back to 1.  This is
not how it should be fixed.


Yeah, the solution might be a bit complicated, but I couldn't find
another way without breaking concept of independent header parsing.
Could you please suggest something if you think that current approach
is wrong? The ticket that this patch tries to fix is 6 years old and
still has discussions going on without any solution.


That's basically why the ticket is still not fixed: there is no
easy fix, and the issue is rather minor, especially compared to
the complexity of the changes required to fix it.


Hmm.. I would argue on the minority of the issue, especially when we talk about
absence of compatibility with RFC and actual discussions in the ticket. Anyway.


While current behaviour might be suboptimal in some use cases, 34it
does not violate RFC, as caching is not something required to
happen.  So we are talking about optimizing some use cases here,
and the same optimization can be easily obtained by adjusting
headers in the backend response.


IMHO, the most promising approach would be to move
Cache-Control/Exires/X-Accel-Expires handling elsewhere, and
handle them somewhere in ngx_http_upstream_process_headers(),
probably along with ngx_http_file_cache_valid() as well.


Ok, cool, I updated my patch, now it's based on the work from Yugo that was left
without comments and is following the way you suggest. Take a look at v2, 
please.


Apart multiple style issues, the patch fails to address the
X-Accel-Expires vs. Cache-Control order issue as outlined in the
ticket, that is, the response with:

: X-Accel-Expires: 10
: Cache-Control: max-age=100, stale-while-revalidate=1000

results in different behaviour than the one with:

: Cache-Control: max-age=100, stale-while-revalidate=1000
: X-Accel-Expires: 10

It is also seems to be completely unreadable.  (Yugo's patch tried
to address this, though had multiple style and readability issues
as well.)


___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

[Vadim Fedorenko via nginx-devel]

Hi Yugo!

I will disagree with your comment. If X-Accel-Expires has value 0 then 
r->cacheable will be zeroed and no caching will occur anyway and the valid_sec 
will never be checked. Overall the patch looks like a simplified version of what 
we already had, which is great!


Best wishes,
Vadim

On 25.04.2022 13:27, Yugo Horie wrote:

Hello,
I appriciate deeply with two of your cooperation.

Maxim's patch looks good to see but it seems to be a bit weird about the 
behavior of the r->cache->valid_sec.
In case of the `X-Accel-Expires: 0` header comes first than the `Cache-Control: 
max-age=10` header, the r->cache->valid_sec could not be overwritten by the 
process_accel_expires which has a weird switch (if it is case 0, it would be 
fall through and return NGX_OK) statements, and then it would be overwritten by 
the process_cache_control(thus Cache-Control:max-age). In other word, it cannot 
kick out the max-age parsing with your brand new extension flag because it 
cannot clear if statements of r->cache-valid_sec !=0.


In contrasts, when the Cache-Control is first, it would be overwritten by 
X-Accel-Expires. I consider this is the right behavior.


Thanks,
Yugo Horie

On Mon, Apr 25, 2022 at 0:43 Maxim Dounin > wrote:


Hello!

On Sun, Apr 24, 2022 at 07:55:17AM +0300, Maxim Dounin wrote:

[...]

 > As far as I can tell, proper behaviour, assuming we parse cache
 > control extensions independently of X-Accel-Expires, can be
 > implemented by using just one flag.

No, that's wrong, as with just one flag it wouldn't be possible to
correctly disable caching of responses with:

Cache-Control: private
Cache-Control: max-age=10

So it needs at least two flags.  Updated patch below, review and
testing appreciated.

# HG changeset patch
# User Maxim Dounin mailto:mdou...@mdounin.ru>>
# Date 1650814681 -10800
#      Sun Apr 24 18:38:01 2022 +0300
# Node ID 940ba4317a97c72d1ee6700cbf58a543fee04c7a
# Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Upstream: fixed X-Accel-Expires/Cache-Control/Expires handling.

Previously, if caching was disabled due to Expires in the past, nginx
failed to cache the response even if it was cacheable as per subsequently
parsed Cache-Control header (ticket #964).

Similarly, if caching was disabled due to Expires in the past,
"Cache-Control: no-cache" or "Cache-Control: max-age=0", caching was not
used if it was cacheable as per subsequently parsed X-Accel-Expires header.

Fix is to avoid disabling caching immediately after parsing Expires in
the past or Cache-Control, but rather set flags which are later checked by
ngx_http_upstream_process_headers() (and cleared by "Cache-Control: max-age"
and X-Accel-Expires).

Additionally, now X-Accel-Expires does not prevent parsing of cache control
extensions, notably stale-while-revalidate and stale-if-error.  This
ensures that order of the X-Accel-Expires and Cache-Control headers is not
important.

Prodded by Vadim Fedorenko and Yugo Horie.

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
@@ -2697,6 +2697,10 @@ ngx_http_upstream_intercept_errors(ngx_h

              if (r->cache) {

+                if (u->headers_in.no_cache || u->headers_in.expired) {
+                    u->cacheable = 0;
+                }
+
                  if (u->cacheable) {
                      time_t  valid;

@@ -2791,6 +2795,10 @@ ngx_http_upstream_process_headers(ngx_ht

      umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);

+    if (u->headers_in.no_cache || u->headers_in.expired) {
+        u->cacheable = 0;
+    }
+
      if (u->headers_in.x_accel_redirect
          && !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))
      {
@@ -4735,18 +4743,18 @@ ngx_http_upstream_process_cache_control(
          return NGX_OK;
      }

-    if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) 
{
-        return NGX_OK;
-    }
-
      start = h->value.data;
      last = start + h->value.len;

+    if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) 
{
+        goto extensions;
+    }
+
      if (ngx_strlcasestrn(start, last, (u_char *) "no-cache", 8 - 1) != 
NULL
          || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != 
NULL
          || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != 
NULL)
      {
-        u->cacheable = 0;
+        u->headers_in.no_cache = 1;
          return NGX_OK;
      }

@@ -4776,12 +4784,15 @@ ngx_http_upstream_process_cache_control(
          }

          if (n == 0) {
-            u->cacheable = 0;
+       

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

[Vadim Fedorenko via nginx-devel]

Hi Maxim!

Thanks for the update, I will make some production tests soon but for now I can 
say that tests from test-suit are passing which is great!


Best wishes,
Vadim

On 24.04.2022 16:42, Maxim Dounin wrote:

Hello!

On Sun, Apr 24, 2022 at 07:55:17AM +0300, Maxim Dounin wrote:

[...]


As far as I can tell, proper behaviour, assuming we parse cache
control extensions independently of X-Accel-Expires, can be
implemented by using just one flag.


No, that's wrong, as with just one flag it wouldn't be possible to
correctly disable caching of responses with:

Cache-Control: private
Cache-Control: max-age=10

So it needs at least two flags.  Updated patch below, review and
testing appreciated.

# HG changeset patch
# User Maxim Dounin 
# Date 1650814681 -10800
#  Sun Apr 24 18:38:01 2022 +0300
# Node ID 940ba4317a97c72d1ee6700cbf58a543fee04c7a
# Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Upstream: fixed X-Accel-Expires/Cache-Control/Expires handling.

Previously, if caching was disabled due to Expires in the past, nginx
failed to cache the response even if it was cacheable as per subsequently
parsed Cache-Control header (ticket #964).

Similarly, if caching was disabled due to Expires in the past,
"Cache-Control: no-cache" or "Cache-Control: max-age=0", caching was not
used if it was cacheable as per subsequently parsed X-Accel-Expires header.

Fix is to avoid disabling caching immediately after parsing Expires in
the past or Cache-Control, but rather set flags which are later checked by
ngx_http_upstream_process_headers() (and cleared by "Cache-Control: max-age"
and X-Accel-Expires).

Additionally, now X-Accel-Expires does not prevent parsing of cache control
extensions, notably stale-while-revalidate and stale-if-error.  This
ensures that order of the X-Accel-Expires and Cache-Control headers is not
important.

Prodded by Vadim Fedorenko and Yugo Horie.

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
@@ -2697,6 +2697,10 @@ ngx_http_upstream_intercept_errors(ngx_h
  
  if (r->cache) {
  
+if (u->headers_in.no_cache || u->headers_in.expired) {

+u->cacheable = 0;
+}
+
  if (u->cacheable) {
  time_t  valid;
  
@@ -2791,6 +2795,10 @@ ngx_http_upstream_process_headers(ngx_ht
  
  umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);
  
+if (u->headers_in.no_cache || u->headers_in.expired) {

+u->cacheable = 0;
+}
+
  if (u->headers_in.x_accel_redirect
  && !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))
  {
@@ -4735,18 +4743,18 @@ ngx_http_upstream_process_cache_control(
  return NGX_OK;
  }
  
-if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {

-return NGX_OK;
-}
-
  start = h->value.data;
  last = start + h->value.len;
  
+if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {

+goto extensions;
+}
+
  if (ngx_strlcasestrn(start, last, (u_char *) "no-cache", 8 - 1) != NULL
  || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL
  || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
  {
-u->cacheable = 0;
+u->headers_in.no_cache = 1;
  return NGX_OK;
  }
  
@@ -4776,12 +4784,15 @@ ngx_http_upstream_process_cache_control(

  }
  
  if (n == 0) {

-u->cacheable = 0;
+u->headers_in.no_cache = 1;
  return NGX_OK;
  }
  
  r->cache->valid_sec = ngx_time() + n;

-}
+u->headers_in.expired = 0;
+}
+
+extensions:
  
  p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",

   23 - 1);
@@ -4863,7 +4874,7 @@ ngx_http_upstream_process_expires(ngx_ht
  expires = ngx_parse_http_time(h->value.data, h->value.len);
  
  if (expires == NGX_ERROR || expires < ngx_time()) {

-u->cacheable = 0;
+u->headers_in.expired = 1;
  return NGX_OK;
  }
  
@@ -4914,6 +4925,8 @@ ngx_http_upstream_process_accel_expires(
  
  default:

  r->cache->valid_sec = ngx_time() + n;
+u->headers_in.no_cache = 0;
+u->headers_in.expired = 0;
  return NGX_OK;
  }
  }
@@ -4925,6 +4938,8 @@ ngx_http_upstream_process_accel_expires(
  
  if (n != NGX_ERROR) {

  r->cache->valid_sec = n;
+u->headers_in.no_cache = 0;
+u->headers_in.expired = 0;
  }
  }
  #endif
diff --git a/src/http/ngx_http_upstream.h b/src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h
+++ b/src/http/ngx_http_upstream.h
@@ -297,6 +297,8 @@ typedef struct {
  
  unsigned connection_close:1;

  unsigned chunked:1;
+  

[PATCH 1 of 4] Core: use explicit_bzero if possible

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1681771172 -10800
#  Tue Apr 18 01:39:32 2023 +0300
# Node ID 0a1c8cb5c05141f3ea3135d9f01688f7693fc7df
# Parent  252a7acd35ceff4fca7a8c60a9aa6d4d22b688bf
Core: use explicit_bzero if possible.

GCC 11+ expanded the scope of dead store elimination optimization
and memory barrier trick doesn't work anymore. But there is new
function exists in glibc to explicitly clear the buffer -
explicit_bzero(). Let's use it instead.
---
 auto/unix | 10 ++
 src/core/ngx_string.c |  4 
 src/core/ngx_string.h |  5 -
 3 files changed, 18 insertions(+), 1 deletion(-)

diff -r 252a7acd35ce -r 0a1c8cb5c051 auto/unix
--- a/auto/unix Mon Apr 17 14:08:00 2023 +0400
+++ b/auto/unix Tue Apr 18 01:39:32 2023 +0300
@@ -1002,3 +1002,13 @@
   if (getaddrinfo("localhost", NULL, NULL, &res) != 0) return 
1;
   freeaddrinfo(res)'
 . auto/feature
+
+
+ngx_feature="explicit_bzero()"
+ngx_feature_name="NGX_HAVE_EXPLICIT_BZERO"
+ngx_feature_run=no
+ngx_feature_incs='#include '
+ngx_feature_path=
+ngx_feature_libs=
+ngx_feature_test="char p[16]; explicit_bzero(p, sizeof(p));"
+. auto/feature
diff -r 252a7acd35ce -r 0a1c8cb5c051 src/core/ngx_string.c
--- a/src/core/ngx_string.c Mon Apr 17 14:08:00 2023 +0400
+++ b/src/core/ngx_string.c Tue Apr 18 01:39:32 2023 +0300
@@ -2080,6 +2080,8 @@
 }
 
 
+#if !(NGX_HAVE_EXPLICIT_BZERO)
+
 void
 ngx_explicit_memzero(void *buf, size_t n)
 {
@@ -2087,6 +2089,8 @@
 ngx_memory_barrier();
 }
 
+#endif
+
 
 #if (NGX_MEMCPY_LIMIT)
 
diff -r 252a7acd35ce -r 0a1c8cb5c051 src/core/ngx_string.h
--- a/src/core/ngx_string.h Mon Apr 17 14:08:00 2023 +0400
+++ b/src/core/ngx_string.h Tue Apr 18 01:39:32 2023 +0300
@@ -87,8 +87,11 @@
  */
 #define ngx_memzero(buf, n)   (void) memset(buf, 0, n)
 #define ngx_memset(buf, c, n) (void) memset(buf, c, n)
-
+#if (NGX_HAVE_EXPLICIT_BZERO)
+#define ngx_explicit_memzero(buf, n)  explicit_bzero(buf, n)
+#else
 void ngx_explicit_memzero(void *buf, size_t n);
+#endif
 
 
 #if (NGX_MEMCPY_LIMIT)
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH 4 of 4] inet: use explicit memzero to avoid optimizations

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1681771255 -10800
#  Tue Apr 18 01:40:55 2023 +0300
# Node ID 460c71c36b00fdd510cb511a5714face68280dac
# Parent  5663d8ff4399e7e76369c024db59c40178290213
inet: use explicit memzero to avoid optimizations.

GCC11+ removes memzero call in ngx_inet6_addr(). Use
ngx_explicit_memzero() to avoid optimization.
---
 src/core/ngx_inet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -r 5663d8ff4399 -r 460c71c36b00 src/core/ngx_inet.c
--- a/src/core/ngx_inet.c   Tue Apr 18 01:40:20 2023 +0300
+++ b/src/core/ngx_inet.c   Tue Apr 18 01:40:55 2023 +0300
@@ -163,7 +163,7 @@
 while (s >= zero) {
 *d-- = *s--;
 }
-ngx_memzero(zero, n);
+ngx_explicit_memzero(zero, n);
 return NGX_OK;
 }
 
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH 2 of 4] md5: use explicit memzero to avoid optimizations

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1681771200 -10800
#  Tue Apr 18 01:40:00 2023 +0300
# Node ID 8f8773a3076bdbd91fc7a4e96d7a068f7ff29b09
# Parent  0a1c8cb5c05141f3ea3135d9f01688f7693fc7df
md5: use explicit memzero to avoid optimizations.

GCC11 is optimizing memzero functions in md5 implementation.
Use ngx_explicit_memzero() instead.
---
 src/core/ngx_md5.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -r 0a1c8cb5c051 -r 8f8773a3076b src/core/ngx_md5.c
--- a/src/core/ngx_md5.cTue Apr 18 01:39:32 2023 +0300
+++ b/src/core/ngx_md5.cTue Apr 18 01:40:00 2023 +0300
@@ -70,13 +70,13 @@
 free = 64 - used;
 
 if (free < 8) {
-ngx_memzero(&ctx->buffer[used], free);
+ngx_explicit_memzero(&ctx->buffer[used], free);
 (void) ngx_md5_body(ctx, ctx->buffer, 64);
 used = 0;
 free = 64;
 }
 
-ngx_memzero(&ctx->buffer[used], free - 8);
+ngx_explicit_memzero(&ctx->buffer[used], free - 8);
 
 ctx->bytes <<= 3;
 ctx->buffer[56] = (u_char) ctx->bytes;
@@ -107,7 +107,7 @@
 result[14] = (u_char) (ctx->d >> 16);
 result[15] = (u_char) (ctx->d >> 24);
 
-ngx_memzero(ctx, sizeof(*ctx));
+ngx_explicit_memzero(ctx, sizeof(*ctx));
 }
 
 
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH 3 of 4] sha1: use explicit memzero to avoid optimizations

[Vadim Fedorenko via nginx-devel]

# HG changeset patch
# User Vadim Fedorenko 
# Date 1681771220 -10800
#  Tue Apr 18 01:40:20 2023 +0300
# Node ID 5663d8ff4399e7e76369c024db59c40178290213
# Parent  8f8773a3076bdbd91fc7a4e96d7a068f7ff29b09
sha1: use explicit memzero to avoid optimizations.

GCC11 is optimizing memzero functions in sha1 implementation.
Use ngx_explicit_memzero() instead.
---
 src/core/ngx_sha1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -r 8f8773a3076b -r 5663d8ff4399 src/core/ngx_sha1.c
--- a/src/core/ngx_sha1.c   Tue Apr 18 01:40:00 2023 +0300
+++ b/src/core/ngx_sha1.c   Tue Apr 18 01:40:20 2023 +0300
@@ -72,13 +72,13 @@
 free = 64 - used;
 
 if (free < 8) {
-ngx_memzero(&ctx->buffer[used], free);
+ngx_explicit_memzero(&ctx->buffer[used], free);
 (void) ngx_sha1_body(ctx, ctx->buffer, 64);
 used = 0;
 free = 64;
 }
 
-ngx_memzero(&ctx->buffer[used], free - 8);
+ngx_explicit_memzero(&ctx->buffer[used], free - 8);
 
 ctx->bytes <<= 3;
 ctx->buffer[56] = (u_char) (ctx->bytes >> 56);
@@ -113,7 +113,7 @@
 result[18] = (u_char) (ctx->e >> 8);
 result[19] = (u_char) ctx->e;
 
-ngx_memzero(ctx, sizeof(*ctx));
+ngx_explicit_memzero(ctx, sizeof(*ctx));
 }
 
 
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH 0 of 4] Avoid dead store elimination in GCC 11+

[Vadim Fedorenko via nginx-devel]

GCC version 11 and newer use more aggressive way to eliminate dead stores
which ends up removing ngx_memzero() calls in several places. Such optimization
affects calculations of md5 and sha1 implemented internally in nginx. The
effect could be easily observed by adding a random data to buffer array in
md5_init() or sha1_init() functions. With this simple modifications the result
of the hash computation will be different each time even though the provided
data to hash is not changed. Changing the code to use current implementation
of ngx_explicit_memzero() doesn't help because of link-time optimizations
enabled in RHEL 9 and derivatives. Glibc 2.34 found in RHEL 9 provides
explicit_bzero() function which should be used to avoid such optimization.
ngx_explicit_memzero() is changed to use explicit_bzero() if possible.
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 0 of 4] Avoid dead store elimination in GCC 11+

[Vadim Fedorenko via nginx-devel]

On 18.04.2023 02:54, Maxim Dounin wrote:

Hello!

On Tue, Apr 18, 2023 at 02:07:06AM +0300, Vadim Fedorenko via nginx-devel wrote:


GCC version 11 and newer use more aggressive way to eliminate dead stores
which ends up removing ngx_memzero() calls in several places. Such optimization
affects calculations of md5 and sha1 implemented internally in nginx. The
effect could be easily observed by adding a random data to buffer array in
md5_init() or sha1_init() functions. With this simple modifications the result
of the hash computation will be different each time even though the provided
data to hash is not changed.


If calculations of md5 and sha1 are affected, this means that the
stores in question are not dead, and they shouldn't be eliminated
in the first place.  From your description this looks like a bug
in the compiler in question.


Yeah, these ngx_memzero()s must not be dead, but according to the standart they
are. In md5_final() the function is called this way: 
ngx_memzero(&ctx->buffer[used], free - 8);

That means that a new variable of type 'char *' is created with the life time
scoped to the call to ngx_memzero(). As the result of of the function is ignored 
explicitly, no other parameters are passed by pointer, and the variable is not 
accessed anywhere else, the whole call can be optimized out.



Alternatively, this can be a bug in nginx code which makes the
compiler think that it can eliminate these ngx_memzero() calls - for
example, GCC is known to do such things if it sees an undefined
behaviour in the code.


There is no undefined behavior unfortunately, everything in this place is well 
defined.



You may want to elaborate more on how to reproduce this, and, if
possible, how to build a minimal test case which demonstrates the
problem.


Sure, let's elaborate a bit. To reproduce the bug you can simply apply the diff:

diff --git a/src/core/ngx_md5.c b/src/core/ngx_md5.c
index c25d0025d..67cc06438 100644
--- a/src/core/ngx_md5.c
+++ b/src/core/ngx_md5.c
@@ -24,6 +24,7 @@ ngx_md5_init(ngx_md5_t *ctx)
 ctx->d = 0x10325476;

 ctx->bytes = 0;
+getrandom(ctx->buffer, 64, 0);
 }


This code will emulate the garbage for the stack-allocated 'ngx_md5_t md5;' in 
ngx_http_file_cache_create_key when nginx is running under the load. Then you 
can use simple configuration:


upstream test_001_origin {
  server 127.0.0.1:8000;
}

proxy_cache_path /var/cache/nginx/test-001 keys_zone=test_001:10m max_size=5g 
inactive=24h levels=1:2 use_temp_path=off;


server {
  listen 127.0.0.1:8000;

  location = /plain {
return 200;
  }

}

server {
  listen 127.0.0.1:80;

  location /oplain {
 proxy_cache test_001;
 proxy_cache_key /oplain;
 proxy_pass http://test_001_origin/plain/;
  }
}


Every time you call 'curl http://127.0.0.1/oplain'  a new cache file will be 
created, but the md5sum of the file will be the same, meaining that the key 
stored in the file is absolutely the same.



Changing the code to use current implementation
of ngx_explicit_memzero() doesn't help because of link-time optimizations
enabled in RHEL 9 and derivatives. Glibc 2.34 found in RHEL 9 provides
explicit_bzero() function which should be used to avoid such optimization.
ngx_explicit_memzero() is changed to use explicit_bzero() if possible.


The ngx_explicit_memzero() function is to be used when zeroed data
are indeed not used afterwards, for example, to make sure
passwords are actually eliminated from memory.  It shouldn't be
used instead of a real ngx_memzero() call - doing so might hide
the problem, which is either in the compiler or in nginx, but
won't fix it.


In this case the nginx code should be fixed to avoid partial memory fillings, 
but such change will come with performance penalty, especially on the CPUs 
without proper `REP MOVSB/MOVSD/MOVSQ` implementation. Controlled usage of 
explicit zeroing is much better is this case.



As for using explicit_bzero() for it, we've looked into various
OS-specific solutions, though there are too many variants out
there, so it was decided that having our own simple implementation
is a better way to go.  If it doesn't work in the particular
setup, it might make sense to adjust our implementation - but
given the above, it might be the same issue which causes the
original problem.


Unfortunately, the memory barrier trick is not working anymore for linker-time 
optimizations. Linker has better information about whether the stored 
information is used again or not. And it will remove memset in such 
implementation, and it will definitely affected security-related code you 
mentioned above. explicit_bzero() function is available in well-loved *BSD 
systems now and is a proper way to do cleaning of the artifacts, doesn't matter 
which implementation is used in the specific system.

___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 0 of 4] Avoid dead store elimination in GCC 11+

[Vadim Fedorenko via nginx-devel]

Hi!

On 18.04.2023 20:14, Maxim Dounin wrote:

Hello!

On Tue, Apr 18, 2023 at 10:50:01AM +0100, Vadim Fedorenko wrote:


On 18.04.2023 02:54, Maxim Dounin wrote:

Hello!

On Tue, Apr 18, 2023 at 02:07:06AM +0300, Vadim Fedorenko via nginx-devel wrote:


GCC version 11 and newer use more aggressive way to eliminate dead stores
which ends up removing ngx_memzero() calls in several places. Such optimization
affects calculations of md5 and sha1 implemented internally in nginx. The
effect could be easily observed by adding a random data to buffer array in
md5_init() or sha1_init() functions. With this simple modifications the result
of the hash computation will be different each time even though the provided
data to hash is not changed.


If calculations of md5 and sha1 are affected, this means that the
stores in question are not dead, and they shouldn't be eliminated
in the first place.  From your description this looks like a bug
in the compiler in question.


Yeah, these ngx_memzero()s must not be dead, but according to the standart they
are. In md5_final() the function is called this way:
ngx_memzero(&ctx->buffer[used], free - 8);
That means that a new variable of type 'char *' is created with the life time
scoped to the call to ngx_memzero(). As the result of of the function is ignored
explicitly, no other parameters are passed by pointer, and the variable is not
accessed anywhere else, the whole call can be optimized out.


The pointer is passed to the function, and the function modifies
the memory being pointed to by the pointer.  While the pointer is
not used anywhere else and can be optimized out, the memory it
points to is used elsewhere, and this modification cannot be
optimized out, so it is incorrect to remove the call according to
my understanding of the C standard.

If you still think it's correct and based on the C standard,
please provide relevant references (and quotes) which explain why
these calls can be optimized out.


Alternatively, this can be a bug in nginx code which makes the
compiler think that it can eliminate these ngx_memzero() calls - for
example, GCC is known to do such things if it sees an undefined
behaviour in the code.


There is no undefined behavior unfortunately, everything in this place is well
defined.


Well, I don't think so.  There is a function call, and it cannot
be eliminated by the compiler unless the compiler thinks that the
results of the function call do not affect the program execution
as externally observed.  Clearly the program execution is affected
(as per your claim).  This leaves us the two possible
alternatives:

- There is a bug in the compiler, and it incorrectly thinks that
   the function call do not affect the program execution.

- There is a bug in the code, and it triggers undefined behaviour,
   so the compiler might not actually know what happens in the code
   (because it not required to do anything meaningful in case of
   undefined behaviour, and simply assume it should never happen).

Just in case, the actual undefined behaviour might occur in the
ngx_md5_body() function due to strict-aliasing rules being broken
by the optimized GET() macro on platforms without strict alignment
requirements if the original data buffer as provided to
ngx_md5_update() cannot be aliased by uint32_t.  See this commit in
the original repository of the md5 code nginx uses:

https://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/popa3d/popa3d/md5/md5.c.diff?r1=1.14;r2=1.15

But nginx only uses ngx_md5_update() with text buffers, so
strict-aliasing rules aren't broken.


You may want to elaborate more on how to reproduce this, and, if
possible, how to build a minimal test case which demonstrates the
problem.


Sure, let's elaborate a bit. To reproduce the bug you can simply apply the diff:

diff --git a/src/core/ngx_md5.c b/src/core/ngx_md5.c
index c25d0025d..67cc06438 100644
--- a/src/core/ngx_md5.c
+++ b/src/core/ngx_md5.c
@@ -24,6 +24,7 @@ ngx_md5_init(ngx_md5_t *ctx)
   ctx->d = 0x10325476;

   ctx->bytes = 0;
+getrandom(ctx->buffer, 64, 0);
   }



Note that this won't compile, it also needs "#include ".


This code will emulate the garbage for the stack-allocated 'ngx_md5_t md5;' in
ngx_http_file_cache_create_key when nginx is running under the load. Then you
can use simple configuration:

upstream test_001_origin {
server 127.0.0.1:8000;
}

proxy_cache_path /var/cache/nginx/test-001 keys_zone=test_001:10m max_size=5g
inactive=24h levels=1:2 use_temp_path=off;

server {
listen 127.0.0.1:8000;

location = /plain {
  return 200;
}

}

server {
listen 127.0.0.1:80;

location /oplain {
   proxy_cache test_001;
   proxy_cache_key /oplain;
   proxy_pass http://test_001_origin/plain/;
}
}


Every time you call 'curl http://127.0.0.1/oplain'  a new cache file will be
created, but the md5sum of the file will be the same,