On Sat, Nov 23, 2019 at 11:45:10PM +0100, Tim Duesterhus wrote: > This commit removes the explicit checks for `if (err)` before > passing `err` to `memprintf`. `memprintf` already checks itself > whether the `**out*` parameter is `NULL` before doing anything. > This reduces the indentation depth and makes the code more readable, > before there is less boilerplate code. > > Instead move the check into the ternary conditional when the error > message should be appended to a previous message. This is consistent > with the rest of ssl_sock.c and with the rest of HAProxy. > > Thus this patch is the arguably cleaner fix for issue #374 and builds > upon > 5f1fa7db86c53827c97f8a8c3f5fa75bfcb5be9a and > 8b453912ce9a4e1a3b1329efb2af04d1e470852e > > Additionally it fixes a few places where the check *still* was missing.
I agree that it's cleaner this way, however it then uncovers another issue which is that *if* ever called with a NULL err then it will leak memory. William said in the issue discussion that the functions are not meant to be called with err=NULL but there are checks for this situation at various places which make all this still very confusing, and the function header makes no mention about input/output arguments. What I'd suggest instead as a better and more durable cleanup would be to explicitly mention above the function's prototype that it must not be called with a null err pointer, and remove all "if (err)" or "err &&" tests so that we are consistent across the whole function. This way it will be easier to spot any offender. Because even if right now nobody calls it with a NULL, it suffices to read the first few lines to see the check being done and believe that it's permitted, so we do have a latent bug quietly waiting for a victim to pass by. Cheers, Willy
