From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Missing checks when malloc returns NULL... |
Date: | 2016-08-19 02:18:10 |
Message-ID: | CAB7nPqQhEF854DuLPaqpx+FfrY3YEcqYTVa12YKXMk_ayCtqqQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
malloc-nulls-v3.patch | text/x-patch | 22.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2016-08-19 02:21:03 | Re: errno clobbering in reorderbuffer |
Previous Message | Craig Ringer | 2016-08-19 01:20:56 | Re: [PATCH] bigint txids vs 'xid' type, new txid_recent(bigint) => xid |