Hello Willy (not being rude this time :p),
On Mon, Apr 16, 2018 at 05:01:18PM +0200, Willy Tarreau wrote:
> I agree on the principle, but memprintf(&err, "foo") will set err to NULL
> if there's no more memory. And I personally care a lot about staying rock
> solid even under harsh memory conditions, because it's always when you have
> the most visitors on your site that you have the least memory left and you
> don't want so many witnesses of your lack of RAM. That's why I'm thinking
> that the "out of memory" error message could more or less serve as a real
> indicator of what happened and as a motivation for developers never to use
> it by default (while "internal error" could be tempting on a lazy day).
>
Here are the two patches with the changes you proposed.
Thanks !
--
Aurélien.
>From 29719bded4ff2b96cb4ac258373c01f0be18428b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Mon, 16 Apr 2018 18:50:19 +0200
Subject: [PATCH 1/2] BUG/MINOR: cli: Guard against NULL messages when using
CLI_ST_PRINT_FREE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Some error paths (especially those followed when running out of memory)
can set the error message to NULL. In order to avoid a crash, use a
generic message ("Out of memory") when this case arises.
It should be backported to 1.8.
Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
src/cli.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/cli.c b/src/cli.c
index 018d508d3..965709ec8 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -625,14 +625,20 @@ static void cli_io_handler(struct appctx *appctx)
else
si_applet_cant_put(si);
break;
- case CLI_ST_PRINT_FREE:
- if (cli_output_msg(res, appctx->ctx.cli.err, LOG_ERR, cli_get_severity_output(appctx)) != -1) {
+ case CLI_ST_PRINT_FREE: {
+ const char *msg = appctx->ctx.cli.err;
+
+ if (!msg)
+ msg = "Out of memory.\n";
+
+ if (cli_output_msg(res, msg, LOG_ERR, cli_get_severity_output(appctx)) != -1) {
free(appctx->ctx.cli.err);
appctx->st0 = CLI_ST_PROMPT;
}
else
si_applet_cant_put(si);
break;
+ }
case CLI_ST_CALLBACK: /* use custom pointer */
if (appctx->io_handler)
if (appctx->io_handler(appctx)) {
--
2.11.0
>From a9b9825d3b6257ccd42d2b56827e23fa91d4768c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Mon, 16 Apr 2018 19:02:42 +0200
Subject: [PATCH 2/2] MINOR: cli: Ensure the CLI always outputs an error when
it should
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When using the CLI_ST_PRINT_FREE state, always output something back
if the faulty function did not fill the 'err' variable.
The map/acl code could lead to a crash whereas the SSL code was silently
failing.
Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
src/map.c | 38 ++++++++++++++++++++++++++++----------
src/ssl_sock.c | 5 +++++
2 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/src/map.c b/src/map.c
index 9313dc87e..7953c2a0b 100644
--- a/src/map.c
+++ b/src/map.c
@@ -723,15 +723,21 @@ static int cli_parse_set_map(char **args, struct appctx *appctx, void *private)
return 1;
}
- /* Try to delete the entry. */
+ /* Try to modify the entry. */
err = NULL;
HA_SPIN_LOCK(PATREF_LOCK, &appctx->ctx.map.ref->lock);
if (!pat_ref_set_by_id(appctx->ctx.map.ref, ref, args[4], &err)) {
HA_SPIN_UNLOCK(PATREF_LOCK, &appctx->ctx.map.ref->lock);
- if (err)
+ if (err) {
memprintf(&err, "%s.\n", err);
- appctx->ctx.cli.err = err;
- appctx->st0 = CLI_ST_PRINT_FREE;
+ appctx->ctx.cli.err = err;
+ appctx->st0 = CLI_ST_PRINT_FREE;
+ }
+ else {
+ appctx->ctx.cli.severity = LOG_ERR;
+ appctx->ctx.cli.msg = "Failed to update an entry.\n";
+ appctx->st0 = CLI_ST_PRINT;
+ }
return 1;
}
HA_SPIN_UNLOCK(PATREF_LOCK, &appctx->ctx.map.ref->lock);
@@ -744,10 +750,16 @@ static int cli_parse_set_map(char **args, struct appctx *appctx, void *private)
HA_SPIN_LOCK(PATREF_LOCK, &appctx->ctx.map.ref->lock);
if (!pat_ref_set(appctx->ctx.map.ref, args[3], args[4], &err)) {
HA_SPIN_UNLOCK(PATREF_LOCK, &appctx->ctx.map.ref->lock);
- if (err)
+ if (err) {
memprintf(&err, "%s.\n", err);
- appctx->ctx.cli.err = err;
- appctx->st0 = CLI_ST_PRINT_FREE;
+ appctx->ctx.cli.err = err;
+ appctx->st0 = CLI_ST_PRINT_FREE;
+ }
+ else {
+ appctx->ctx.cli.severity = LOG_ERR;
+ appctx->ctx.cli.msg = "Failed to update an entry.\n";
+ appctx->st0 = CLI_ST_PRINT;
+ }
return 1;
}
HA_SPIN_UNLOCK(PATREF_LOCK, &appctx->ctx.map.ref->lock);
@@ -829,10 +841,16 @@ static int cli_parse_add_map(char **args, struct appctx *appctx, void *private)
ret = pat_ref_add(appctx->ctx.map.ref, args[3], NULL, &err);
HA_SPIN_UNLOCK(PATREF_LOCK, &appctx->ctx.map.ref->lock);
if (!ret) {
- if (err)
+ if (err) {
memprintf(&err, "%s.\n", err);
- appctx->ctx.cli.err = err;
- appctx->st0 = CLI_ST_PRINT_FREE;
+ appctx->ctx.cli.err = err;
+ appctx->st0 = CLI_ST_PRINT_FREE;
+ }
+ else {
+ appctx->ctx.cli.severity = LOG_ERR;
+ appctx->ctx.cli.msg = "Failed to add an entry.\n";
+ appctx->st0 = CLI_ST_PRINT;
+ }
return 1;
}
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 8151cb381..23ad35b18 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -8588,6 +8588,11 @@ static int cli_parse_set_ocspresponse(char **args, struct appctx *appctx, void *
appctx->ctx.cli.err = err;
appctx->st0 = CLI_ST_PRINT_FREE;
}
+ else {
+ appctx->ctx.cli.severity = LOG_ERR;
+ appctx->ctx.cli.msg = "Failed to update OCSP response.\n";
+ appctx->st0 = CLI_ST_PRINT;
+ }
return 1;
}
appctx->ctx.cli.severity = LOG_INFO;
--
2.11.0
