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
