On 10/24/2018 04:49 AM, Willy Tarreau wrote:
Hi Fred,
On Tue, Oct 23, 2018 at 02:57:05PM +0200, Frederic Lecaille wrote:
Hello ML,
Here is a serie of patches to make the cache capable of caching HTTP
objects larger than a buffer.
The 4th patch add "max-object-size" option to "cache" section so that
to limit the size of the HTTP objects to be cached.
Do not hesitate to test them.
Great, thanks, now applied!
I'm just having one concern I'd like you to take a look at. The
max-object-size and the "sent" argument are respectively unsigned int
and int, while the response's body_len is an uint64_t. Thus I have no
idea what happens when :
- max-object-size >= 2 GB is set
- content-length >= 2 GB or >= 4 GB is seen
- the sum of received chunks is larger than 2 GB
Well, after having checked, haproxy could start with a cache bigger than
2047 MB on my PC due to parsing issue.
I provide three patches. The first fixes the "total-max-size" parsing
issue. The second patch is there to also parse "max-object-size" as an
unsigned int to avoid weird issues (implicit conversions). The last if
for the documentation update.
Note that the maximum value of "max-object-size" is 4095/2 MB which may
be stored as an int.
I'm personally fine if all of these result in the object not being cached
(in which case it should be mentioned in the doc), but I'd like to be sure
we don't return truncated/corrupted objects and that we don't leak cache
entries or any such thing.
Thanks!
Willy
>From d9409e253dce6b0041f547645949de07b51b615d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <[email protected]>
Date: Thu, 25 Oct 2018 10:46:40 +0200
Subject: [PATCH 3/3] DOC: cache: Missing information about "total-max-size"
and "max-object-size"
---
doc/configuration.txt | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 4431e833..95b0b977 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -17493,11 +17493,12 @@ cache <name>
total-max-size <megabytes>
Define the size in RAM of the cache in megabytes. This size is split in
- blocks of 1kB which are used by the cache entries.
+ blocks of 1kB which are used by the cache entries. Its maximum value is 4095.
max-object-size <bytes>
- Define the maximum size of the objects to be cached. If not set, it equals
- to a 256th of the cache size.
+ Define the maximum size of the objects to be cached. Must not be greater than
+ an half of "total-max-size". If not set, it equals to a 256th of the cache size.
+ All objects with sizes larger than "max-object-size" will not be cached.
max-age <seconds>
Define the maximum expiration duration. The expiration is set has the lowest
--
2.11.0
>From 947047b4675cfed827c4fa9bac800f18e1d7abce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <[email protected]>
Date: Thu, 25 Oct 2018 10:39:49 +0200
Subject: [PATCH 2/3] MINOR: cache: Avoid usage of atoi() when parsing
"max-object-size".
With this patch we avoid parsing "max-object-size" with atoi() and
we store its value as an unsigned int to prevent bad implicit conversion issues
especially when we compare it with others unsigned value (content length).
---
include/proto/shctx.h | 3 ++-
include/types/shctx.h | 2 +-
src/cache.c | 22 +++++++++++++++++++---
src/shctx.c | 4 ++--
4 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/include/proto/shctx.h b/include/proto/shctx.h
index 594a81d5..9fc6fad8 100644
--- a/include/proto/shctx.h
+++ b/include/proto/shctx.h
@@ -32,7 +32,8 @@
#endif
int shctx_init(struct shared_context **orig_shctx,
- int maxblocks, int blocksize, int maxobjsz, int extra, int shared);
+ int maxblocks, int blocksize, unsigned int maxobjsz,
+ int extra, int shared);
struct shared_block *shctx_row_reserve_hot(struct shared_context *shctx,
struct shared_block *last, int data_len);
void shctx_row_inc_hot(struct shared_context *shctx, struct shared_block *first);
diff --git a/include/types/shctx.h b/include/types/shctx.h
index 53dca3f1..7d9d8c8a 100644
--- a/include/types/shctx.h
+++ b/include/types/shctx.h
@@ -40,7 +40,7 @@ struct shared_context {
struct list avail; /* list for active and free blocks */
struct list hot; /* list for locked blocks */
unsigned int nbav; /* number of available blocks */
- int max_obj_size; /* maximum object size. */
+ unsigned int max_obj_size; /* maximum object size (in bytes). */
void (*free_block)(struct shared_block *first, struct shared_block *block);
short int block_size;
unsigned char data[0];
diff --git a/src/cache.c b/src/cache.c
index 77986fb9..b9ac2d50 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -50,7 +50,7 @@ struct cache {
struct eb_root entries; /* head of cache entries based on keys */
unsigned int maxage; /* max-age */
unsigned int maxblocks;
- unsigned int maxobjsz; /* max-object-size */
+ unsigned int maxobjsz; /* max-object-size (in bytes) */
char id[33]; /* cache name */
};
@@ -878,6 +878,9 @@ int cfg_parse_cache(const char *file, int linenum, char **args, int kwm)
tmp_cache_config->maxage = atoi(args[1]);
} else if (strcmp(args[0], "max-object-size") == 0) {
+ unsigned int maxobjsz;
+ char *err;
+
if (alertif_too_many_args(1, file, linenum, args, &err_code)) {
err_code |= ERR_ABORT;
goto out;
@@ -889,7 +892,14 @@ int cfg_parse_cache(const char *file, int linenum, char **args, int kwm)
err_code |= ERR_WARN;
}
- tmp_cache_config->maxobjsz = atoi(args[1]);
+ maxobjsz = strtoul(args[1], &err, 10);
+ if (err == args[1] || *err != '\0') {
+ ha_warning("parsing [%s:%d]: max-object-size wrong value '%s'\n",
+ file, linenum, args[1]);
+ err_code |= ERR_ABORT;
+ goto out;
+ }
+ tmp_cache_config->maxobjsz = maxobjsz;
}
else if (*args[0] != 0) {
ha_alert("parsing [%s:%d] : unknown keyword '%s' in 'cache' section\n", file, linenum, args[0]);
@@ -917,10 +927,16 @@ int cfg_post_parse_section_cache()
goto out;
}
- if (!tmp_cache_config->maxobjsz)
+ if (!tmp_cache_config->maxobjsz) {
/* Default max. file size is a 256th of the cache size. */
tmp_cache_config->maxobjsz =
(tmp_cache_config->maxblocks * CACHE_BLOCKSIZE) >> 8;
+ }
+ else if (tmp_cache_config->maxobjsz > tmp_cache_config->maxblocks * CACHE_BLOCKSIZE / 2) {
+ ha_alert("\"max-object-size\" is limited to an half of \"total-max-size\" => %u\n", tmp_cache_config->maxblocks * CACHE_BLOCKSIZE / 2);
+ err_code |= ERR_FATAL | ERR_ALERT;
+ goto out;
+ }
ret_shctx = shctx_init(&shctx, tmp_cache_config->maxblocks, CACHE_BLOCKSIZE,
tmp_cache_config->maxobjsz, sizeof(struct cache), 1);
diff --git a/src/shctx.c b/src/shctx.c
index 604fd7df..9fe12e81 100644
--- a/src/shctx.c
+++ b/src/shctx.c
@@ -292,7 +292,7 @@ int shctx_row_data_get(struct shared_context *shctx, struct shared_block *first,
* and 0 if cache is already allocated.
*/
int shctx_init(struct shared_context **orig_shctx, int maxblocks, int blocksize,
- int maxobjsz, int extra, int shared)
+ unsigned int maxobjsz, int extra, int shared)
{
int i;
struct shared_context *shctx;
@@ -359,7 +359,7 @@ int shctx_init(struct shared_context **orig_shctx, int maxblocks, int blocksize,
LIST_INIT(&shctx->hot);
shctx->block_size = blocksize;
- shctx->max_obj_size = maxobjsz;
+ shctx->max_obj_size = maxobjsz == (unsigned int)-1 ? 0 : maxobjsz;
/* init the free blocks after the shared context struct */
cur = (void *)shctx + sizeof(struct shared_context) + extra;
--
2.11.0
>From 7e9993e2b1a59a15504f0501232c65c84db98bf8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <[email protected]>
Date: Wed, 24 Oct 2018 17:53:04 +0200
Subject: [PATCH 1/3] BUG/MINOR: cache: Crashes with "total-max-size" >
2047(MB).
With this patch we support cache size larger than 2047 (MB) and prevent
haproxy from crashing when "total-max-size" is parsed as negative values
by atoi().
The limit at parsing time is 4095 MB (UINT_MAX >> 20).
May be backported to 1.8.
---
src/cache.c | 23 +++++++++++++++++++----
src/ssl_sock.c | 2 +-
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/cache.c b/src/cache.c
index d2411543..77986fb9 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -838,17 +838,32 @@ int cfg_parse_cache(const char *file, int linenum, char **args, int kwm)
tmp_cache_config->maxobjsz = 0;
}
} else if (strcmp(args[0], "total-max-size") == 0) {
- int maxsize;
+ unsigned long int maxsize;
+ char *err;
if (alertif_too_many_args(1, file, linenum, args, &err_code)) {
err_code |= ERR_ABORT;
goto out;
}
+ maxsize = strtoul(args[1], &err, 10);
+ if (err == args[1] || *err != '\0') {
+ ha_warning("parsing [%s:%d]: total-max-size wrong value '%s'\n",
+ file, linenum, args[1]);
+ err_code |= ERR_ABORT;
+ goto out;
+ }
+
+ if (maxsize > (UINT_MAX >> 20)) {
+ ha_warning("parsing [%s:%d]: \"total-max-size\" (%s) must not be greater than %u\n",
+ file, linenum, args[1], UINT_MAX >> 20);
+ err_code |= ERR_ABORT;
+ goto out;
+ }
+
/* size in megabytes */
- maxsize = atoi(args[1]) * 1024 * 1024 / CACHE_BLOCKSIZE;
+ maxsize *= 1024 * 1024 / CACHE_BLOCKSIZE;
tmp_cache_config->maxblocks = maxsize;
-
} else if (strcmp(args[0], "max-age") == 0) {
if (alertif_too_many_args(1, file, linenum, args, &err_code)) {
err_code |= ERR_ABORT;
@@ -910,7 +925,7 @@ int cfg_post_parse_section_cache()
ret_shctx = shctx_init(&shctx, tmp_cache_config->maxblocks, CACHE_BLOCKSIZE,
tmp_cache_config->maxobjsz, sizeof(struct cache), 1);
- if (ret_shctx < 0) {
+ if (ret_shctx <= 0) {
if (ret_shctx == SHCTX_E_INIT_LOCK)
ha_alert("Unable to initialize the lock for the cache.\n");
else
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 140f406b..50af63b2 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -4789,7 +4789,7 @@ int ssl_sock_prepare_bind_conf(struct bind_conf *bind_conf)
sizeof(struct sh_ssl_sess_hdr) + SHSESS_BLOCK_MIN_SIZE, -1,
sizeof(*sh_ssl_sess_tree),
((global.nbthread > 1) || (!global_ssl.private_cache && (global.nbproc > 1))) ? 1 : 0);
- if (alloc_ctx < 0) {
+ if (alloc_ctx <= 0) {
if (alloc_ctx == SHCTX_E_INIT_LOCK)
ha_alert("Unable to initialize the lock for the shared SSL session cache. You can retry using the global statement 'tune.ssl.force-private-cache' but it could increase CPU usage due to renegotiations if nbproc > 1.\n");
else
--
2.11.0
