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

Reply via email to