Welcome! Log In Create A New Profile

Advanced

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

Posted by 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.
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(&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;

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
On Mon, Apr 16, 2018 at 4:19 PM, Willy Tarreau <[email protected]> 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(&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;
>
> 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
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 <[email protected]> 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(&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).

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

No problem :-)

Willy
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.
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)
Sorry, only registered users may post in this forum.

Click here to login