On Mon, Apr 16, 2018 at 4:19 PM, Willy Tarreau <w...@1wt.eu> 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

Reply via email to