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 <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.
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
