On 10.09.2015 17:33, Maxim Dounin wrote:
Hello!
On Thu, Sep 10, 2015 at 05:07:36PM +0200, Sergey Brester wrote:
Leave header format unchanged (I mean changes in header file
'src/http/ngx_http_cache.h'), but not calculate and not compare crc32
(unused / reserved up to "change cache header format next time")?
Even that way resulting cache files will not be compatible with
older versions which do the check, thus breaking upgrades (when
cache items can be used by different versions simultaneously for a
short time) and, more importantly, downgrades (which sometimes
happen due to various reasons).
In this case (someone will downgrade back to previous nginx version) the
cache entries will be invalidated by first use (because crc32 will not
equal) - well I think it's not a great problem.
Please find enclosed a changeset (2nd) that should restore backwards
compatibility of cache header file (forwards).
Regards,
sebres.
# HG changeset patch
# User sebres <serg.bres...@sebres.de>
# Date 1441885669 -7200
# Thu Sep 10 13:47:49 2015 +0200
# Node ID 4293b50a7977e86c98bd9ae245dc03156a0a8888
# Parent f829cfb5364c0f32eae7a608c18cab1dc9b06a87
always compares keys of cache entry (hash is insufficient secure, so protects in case of hash collision),
see "http://forum.nginx.org/read.php?29,261413,261529"; for a discussion about;
additional protection via crc32 is obsolete and removed now;
diff -r f829cfb5364c -r 4293b50a7977 src/http/ngx_http_cache.h
--- a/src/http/ngx_http_cache.h Thu Sep 03 15:09:21 2015 +0300
+++ b/src/http/ngx_http_cache.h Thu Sep 10 13:47:49 2015 +0200
@@ -64,7 +64,6 @@ typedef struct {
struct ngx_http_cache_s {
ngx_file_t file;
ngx_array_t keys;
- uint32_t crc32;
u_char key[NGX_HTTP_CACHE_KEY_LEN];
u_char main[NGX_HTTP_CACHE_KEY_LEN];
@@ -119,7 +118,6 @@ typedef struct {
time_t valid_sec;
time_t last_modified;
time_t date;
- uint32_t crc32;
u_short valid_msec;
u_short header_start;
u_short body_start;
diff -r f829cfb5364c -r 4293b50a7977 src/http/ngx_http_file_cache.c
--- a/src/http/ngx_http_file_cache.c Thu Sep 03 15:09:21 2015 +0300
+++ b/src/http/ngx_http_file_cache.c Thu Sep 10 13:47:49 2015 +0200
@@ -233,7 +233,6 @@ ngx_http_file_cache_create_key(ngx_http_
len = 0;
- ngx_crc32_init(c->crc32);
ngx_md5_init(&md5);
key = c->keys.elts;
@@ -243,14 +242,12 @@ ngx_http_file_cache_create_key(ngx_http_
len += key[i].len;
- ngx_crc32_update(&c->crc32, key[i].data, key[i].len);
ngx_md5_update(&md5, key[i].data, key[i].len);
}
c->header_start = sizeof(ngx_http_file_cache_header_t)
+ sizeof(ngx_http_file_cache_key) + len + 1;
- ngx_crc32_final(c->crc32);
ngx_md5_final(c->key, &md5);
ngx_memcpy(c->main, c->key, NGX_HTTP_CACHE_KEY_LEN);
@@ -521,9 +518,12 @@ wakeup:
static ngx_int_t
ngx_http_file_cache_read(ngx_http_request_t *r, ngx_http_cache_t *c)
{
+ u_char *p;
time_t now;
ssize_t n;
+ ngx_str_t *key;
ngx_int_t rc;
+ ngx_uint_t i;
ngx_http_file_cache_t *cache;
ngx_http_file_cache_header_t *h;
@@ -547,12 +547,28 @@ ngx_http_file_cache_read(ngx_http_reques
return NGX_DECLINED;
}
- if (h->crc32 != c->crc32) {
+ if (h->header_start != c->header_start) {
ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
- "cache file \"%s\" has md5 collision", c->file.name.data);
+ "cache file \"%s\" has hash collision", c->file.name.data);
return NGX_DECLINED;
}
+ p = c->buf->pos + sizeof(ngx_http_file_cache_header_t)
+ + sizeof(ngx_http_file_cache_key);
+
+ /* because any hash is insufficient, check keys are equal also */
+ key = c->keys.elts;
+ for (i = 0; i < c->keys.nelts; i++) {
+ if (ngx_memcmp(p, key[i].data, key[i].len) != 0) {
+ ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
+ "cache file \"%s\" has hash collision",
+ c->file.name.data);
+ return NGX_DECLINED;
+ }
+
+ p += key[i].len;
+ }
+
if ((size_t) h->body_start > c->body_start) {
ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
"cache file \"%s\" has too long header",
@@ -583,7 +599,6 @@ ngx_http_file_cache_read(ngx_http_reques
c->last_modified = h->last_modified;
c->date = h->date;
c->valid_msec = h->valid_msec;
- c->header_start = h->header_start;
c->body_start = h->body_start;
c->etag.len = h->etag_len;
c->etag.data = h->etag;
@@ -1229,7 +1244,6 @@ ngx_http_file_cache_set_header(ngx_http_
h->valid_sec = c->valid_sec;
h->last_modified = c->last_modified;
h->date = c->date;
- h->crc32 = c->crc32;
h->valid_msec = (u_short) c->valid_msec;
h->header_start = (u_short) c->header_start;
h->body_start = (u_short) c->body_start;
@@ -1468,7 +1482,6 @@ ngx_http_file_cache_update_header(ngx_ht
if (h.version != NGX_HTTP_CACHE_VERSION
|| h.last_modified != c->last_modified
- || h.crc32 != c->crc32
|| h.header_start != c->header_start
|| h.body_start != c->body_start)
{
@@ -1489,7 +1502,6 @@ ngx_http_file_cache_update_header(ngx_ht
h.valid_sec = c->valid_sec;
h.last_modified = c->last_modified;
h.date = c->date;
- h.crc32 = c->crc32;
h.valid_msec = (u_short) c->valid_msec;
h.header_start = (u_short) c->header_start;
h.body_start = (u_short) c->body_start;
# HG changeset patch
# User sebres <serg.bres...@sebres.de>
# Date 1441899056 -7200
# Thu Sep 10 17:30:56 2015 +0200
# Node ID 06ebe85589e4646947ff6a56bc35cd992c78f5d6
# Parent 4293b50a7977e86c98bd9ae245dc03156a0a8888
cache header file backwards compatibility: restore crc32 as unused, todo: delete crc32 next time cache header format changed
diff -r 4293b50a7977 -r 06ebe85589e4 src/http/ngx_http_cache.h
--- a/src/http/ngx_http_cache.h Thu Sep 10 13:47:49 2015 +0200
+++ b/src/http/ngx_http_cache.h Thu Sep 10 17:30:56 2015 +0200
@@ -118,6 +118,8 @@ typedef struct {
time_t valid_sec;
time_t last_modified;
time_t date;
+ /* unused - todo: delete crc32 next time cache header format changed */
+ uint32_t crc32;
u_short valid_msec;
u_short header_start;
u_short body_start;
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
