Re: Missing checks when malloc returns NULL...
От | Michael Paquier |
---|---|
Тема | Re: Missing checks when malloc returns NULL... |
Дата | |
Msg-id | CAB7nPqQhEF854DuLPaqpx+FfrY3YEcqYTVa12YKXMk_ayCtqqQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Missing checks when malloc returns NULL... (Heikki Linnakangas <hlinnaka@iki.fi>) |
Ответы |
Re: Missing checks when malloc returns NULL...
(Michael Paquier <michael.paquier@gmail.com>)
|
Список | pgsql-hackers |
On Thu, Aug 18, 2016 at 6:12 PM, Heikki Linnakangas <hlinnaka@iki.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
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Craig RingerДата:
Сообщение: Re: [PATCH] bigint txids vs 'xid' type, new txid_recent(bigint) => xid