Re: Missing checks when malloc returns NULL...

On Thu, Aug 18, 2016 at 6:12 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 06/22/2016 04:41 AM, Michael Paquier wrote:
>> make s
>> OK, there is not much that we can do here then. What about the rest?
>> Those seem like legit concerns to me.
>
>
> There's also a realloc() and an strdup() call in refint.c. But looking at
> refint.c, I wonder why it's using malloc()/free() in the first place, rather
> than e.g. TopMemoryContext. The string construction code with sprintf() and
> strlen() looks quite antiqued, too, StringInfo would make it more readable.
>
> Does refint.c actually have any value anymore? What if we just removed it
> altogether? It's good to have some C triggers in contrib, to serve as
> examples, and to have some regression test coverage for all that. But ISTM
> that the 'autoinc' module covers the trigger API just as well.

I'd be in favor for nuking it instead of keeping this code as it
overlaps with autoinc, I did not notice that. Even if that's an
example, it is actually showing some bad code patters, which kills its
own purpose.

> The code in ps_status() runs before the elog machinery has been initialized,
> so you get a rather unhelpful error:
>
>> error occurred at ps_status.c:167 before error message processing is
>> available
>
> I guess that's still better than outright crashing, though. There are also a
> few strdup() calls there that can run out of memory..

Right. Another possibility is to directly call write_stderr to be sure
that the user gets the right message, then do exit(1). Thinking more
about it, as save_ps_display_args is called really at the beginning
this is perhaps what makes the most sense, so I changed the patch this
way.

> Not many of the callers of simple_prompt() check for a NULL result, so in
> all probability, returning NULL from there will just crash in the caller.
> There's one existing "return NULL" in there already, so this isn't a new
> problem, but can we find a better way?

I got inspired by the return value in the case of WIN32. Letting
sprompt.c in charge of printing an error does not seem like a good
idea to me, and there are already callers of simple_prompt() that
check for NULL and report an error appropriately, like pg_backup_db.c.
So I think that we had better adapt all the existing calls of
simple_prompt checking for NULL and make things more consistent by
having the callers report errors. Hence I updated the patch with those
changes.

Another possibility would be to keep a static buffer that has a fixed
size, basically 4096 as this is the maximum expected by psql, but
that's a waste of bytes in all other cases: one caller uses 4096, two
128 and the rest use 100 or less.

By the way, while looking at that, I also noticed that in exec_command
of psql's command.c we don't check for gets_fromFile that could return
NULL.

> There are more malloc(), realloc() and strdup() calls in isolationtester and
> pg_regress, that we ought to change too while we're at it.

Right. I added some handling for those callers as well with the pg_ equivalents.
--
Michael