Re: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Willy Tarreau]

On Mon, Apr 16, 2018 at 07:19:15PM +0200, Aurélien Nephtali wrote:
> Hello Willy (not being rude this time :p),

Great, now applied, thank you!

Willy
PS: you were not rude (or I didn't sense it at least)

Re: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Aurélien Nephtali]

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(, "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?= 
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 
---
 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?= 
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 
---
 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, >ctx.map.ref->lock);
 			if (!pat_ref_set_by_id(appctx->ctx.map.ref, ref, args[4], )) {
 HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
-if (err)
+if (err) {
 	memprintf(, "%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, >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, >ctx.map.ref->lock);
 			if (!pat_ref_set(appctx->ctx.map.ref, args[3], args[4], )) {
 HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
-if (err)
+if (err) {
 	memprintf(, "%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, >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, );
 		HA_SPIN_UNLOCK(PATREF_LOCK, 

Re: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Willy Tarreau]

On Mon, Apr 16, 2018 at 04:41:27PM +0200, Aurélien Nephtali wrote:
> On Mon, Apr 16, 2018 at 4:19 PM, Willy Tarreau  wrote:
> > Hi Aurélien,
> >
> > On Sun, Apr 15, 2018 at 09:58:49AM +0200, Aurélien Nephtali wrote:
> >> Hello,
> >>
> >> Here is a small patch to fix a potential crash when using
> >> CLI_ST_PRINT_FREE in an error path in the 'map' code.
> >> The problematic part is in the 'add' feature but all other usages have
> >> ben modified the same way to be consistent.
> >
> > Interesting one. In fact, while it does provide a friendlier error message
> > to the user, the real issue in my opinion is in the cli_io_handler() where
> > it handles CLI_ST_PRINT_FREE, where it's not defensive enough against a
> > NULL in appctx->ctx.cli.err. And even with your patch this situation can
> > arise if an out of memory condition happens in the final memprintf() of
> > the map code.
> >
> > Thus what I'd suggest would be instead to check for NULL there and to fall
> > back to a generic "out of memory" error message (if that makes sense, maybe
> > other situations may lead to this, I don't know) as a first patch,
> 
> This was my first idea, using "Internal error" or something like that
> but I had the feeling it was covering some cases that should be
> properly handled.
> As it's "internal code" I bet on the fact that it should not happen.

I agree on the principle, but memprintf(, "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).

> Arg!#@ sorry, I thought I got them all.. My indent rules were reset at
> some point and these lines slipped through.

No problem :-)

Willy

Re: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Aurélien Nephtali]

On Mon, Apr 16, 2018 at 4:19 PM, Willy Tarreau  wrote:
> Hi Aurélien,
>
> On Sun, Apr 15, 2018 at 09:58:49AM +0200, Aurélien Nephtali wrote:
>> Hello,
>>
>> Here is a small patch to fix a potential crash when using
>> CLI_ST_PRINT_FREE in an error path in the 'map' code.
>> The problematic part is in the 'add' feature but all other usages have
>> ben modified the same way to be consistent.
>
> Interesting one. In fact, while it does provide a friendlier error message
> to the user, the real issue in my opinion is in the cli_io_handler() where
> it handles CLI_ST_PRINT_FREE, where it's not defensive enough against a
> NULL in appctx->ctx.cli.err. And even with your patch this situation can
> arise if an out of memory condition happens in the final memprintf() of
> the map code.
>
> Thus what I'd suggest would be instead to check for NULL there and to fall
> back to a generic "out of memory" error message (if that makes sense, maybe
> other situations may lead to this, I don't know) as a first patch,

This was my first idea, using "Internal error" or something like that
but I had the feeling it was covering some cases that should be
properly handled.
As it's "internal code" I bet on the fact that it should not happen.

>From what I saw briefly all errors paths fill 'err' but I may have
overlooked some cases.

> then another one which is just a small improvement to make error messages more
> relevant for map and ocsp (which is exactly what your patch does).
>
> I'm just having a small comment below :
>
>> - if (err)
>> + if (err) {
>>   memprintf(, "%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;
>
> Please be careful above, as you can see, the lines are filled with spaces,
> maybe the code was copy-pasted there (it's the same at other locations).
>

Arg!#@ sorry, I thought I got them all.. My indent rules were reset at
some point and these lines slipped through.
I usually have a rule to show extra tabs when I should use spaces but
not the inverse :).

-- 
Aurélien Nephtali

Re: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Willy Tarreau]

Hi Aurélien,

On Sun, Apr 15, 2018 at 09:58:49AM +0200, Aurélien Nephtali wrote:
> Hello,
> 
> Here is a small patch to fix a potential crash when using
> CLI_ST_PRINT_FREE in an error path in the 'map' code.
> The problematic part is in the 'add' feature but all other usages have
> ben modified the same way to be consistent.

Interesting one. In fact, while it does provide a friendlier error message
to the user, the real issue in my opinion is in the cli_io_handler() where
it handles CLI_ST_PRINT_FREE, where it's not defensive enough against a
NULL in appctx->ctx.cli.err. And even with your patch this situation can
arise if an out of memory condition happens in the final memprintf() of
the map code.

Thus what I'd suggest would be instead to check for NULL there and to fall
back to a generic "out of memory" error message (if that makes sense, maybe
other situations may lead to this, I don't know) as a first patch, then
another one which is just a small improvement to make error messages more
relevant for map and ocsp (which is exactly what your patch does).

I'm just having a small comment below :

> - if (err)
> + if (err) {
>   memprintf(, "%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;

Please be careful above, as you can see, the lines are filled with spaces,
maybe the code was copy-pasted there (it's the same at other locations). 

Thanks!
Willy

[PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Aurélien Nephtali]

Hello,

Here is a small patch to fix a potential crash when using
CLI_ST_PRINT_FREE in an error path in the 'map' code.
The problematic part is in the 'add' feature but all other usages have
ben modified the same way to be consistent.

It also adds a missing error message in the SSL code when the loading of
an OCSP response failed. The error path always sets the 'err' variable
but it's a good idea to handle the case where it does not in the event
that future code forgot to fill it.

-- 
Aurélien.
>From e91962c02cdc0ae48ff59066cdc0f22e1063b496 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Sun, 15 Apr 2018 07:39:54 +0200
Subject: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when
 using CLI_ST_PRINT_FREE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In the CLI, appctx->ctx.cli.err must point to a valid string when using
the state CLI_ST_PRINT_FREE otherwise a crash will happen.

In the map code, one error path does not fill the 'err' variable
(pat_ref_add() -> pat_ref_push()) and in the SSL code it is correct but
it lacks a default error message if the function called does not fill
'err'.

In both cases only use CLI_ST_PRINT_FREE if 'err' is not NULL otherwise
use a static default message.

It should be backported to 1.8.

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..6cf6e4a87 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, >ctx.map.ref->lock);
 			if (!pat_ref_set_by_id(appctx->ctx.map.ref, ref, args[4], )) {
 HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
-if (err)
+if (err) {
 	memprintf(, "%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, >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, >ctx.map.ref->lock);
 			if (!pat_ref_set(appctx->ctx.map.ref, args[3], args[4], )) {
 HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
-if (err)
+if (err) {
 	memprintf(, "%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, >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, );
 		HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
 		if (!ret) {
-			if (err)
+			if (err) {
 memprintf(, "%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