On 10/25/2018 07:01 PM, William Lallemand wrote:
Hi Fred!
On Thu, Oct 25, 2018 at 10:59:43AM +0200, Frederic Lecaille wrote:
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.
Good catch!
Could you split the patches which contains shctx changes? Changes in the shctx
API should have their own patches.
You last patch seems to contain several fixes, 1 on the cache configuration
parsing, and 2 others related to the way we test shctx_init. We mustn't have
patches on ssl_sock.c in a patch related to the cache.
Ok.
Here is a new series of patches.
Fred.
>From 03a27f53943b439cbaa1fe2bda92a57af1db13cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Thu, 25 Oct 2018 10:46:40 +0200
Subject: [PATCH 6/6] 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 6e5445c400b7272006e0d8e2d0f7d1fd14147295 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Thu, 25 Oct 2018 20:31:40 +0200
Subject: [PATCH 5/6] MINOR: shctx: Change max. object size type to unsigned
int.
This change is there to prevent implicit conversions when comparing
shctx maximum object sizes with other unsigned values.
---
include/proto/shctx.h | 3 ++-
include/types/shctx.h | 2 +-
src/shctx.c | 4 ++--
3 files changed, 5 insertions(+), 4 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/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 b12ab6b6922cad7c10dcf9503f97dff15e35d862 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Thu, 25 Oct 2018 20:29:31 +0200
Subject: [PATCH 4/6] 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).
---
src/cache.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
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);
--
2.11.0
>From bcdc8fdac23b638fd6a2263f1c07420da6794706 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Thu, 25 Oct 2018 20:22:46 +0200
Subject: [PATCH 3/6] BUG/MINOR: ssl: Wrong usage of shctx_init().
With this patch we check that shctx_init() does not return 0.
Must be backported to 1.8.
---
src/ssl_sock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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
>From c96cf4326f25e4fb8698370f89bae4c4b2110bab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Thu, 25 Oct 2018 20:18:59 +0200
Subject: [PATCH 2/6] BUG/MINOR: cache: Wrong usage of shctx_init().
With this patch we check that shctx_init() does not returns 0.
This is possible if the maxblocks argument, which is passed as an
int, is negative due to an implicit conversion.
Must be backported to 1.8.
---
src/cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/cache.c b/src/cache.c
index 9070e996..77986fb9 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -925,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
--
2.11.0
>From 06a07ee7ffee5f0ccc797ce9c680d7ec92ae204d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Thu, 25 Oct 2018 20:17:45 +0200
Subject: [PATCH 1/6] 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 | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/cache.c b/src/cache.c
index d2411543..9070e996 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;
--
2.11.0
