Re: Missing checks when malloc returns NULL...

On Sat, Aug 27, 2016 at 12:33 AM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> wrote:

Your patch [1] was marked as "Needs review" so I decided to take a look.

Thanks for the input!

It looks good to me. However I found dozens of places in PostgreSQL code
that seem to have similar problem you are trying to fix [2]. As far as I
understand these are only places left that don't check malloc/realloc/
strdup return values properly. I thought maybe you will be willing to
fix they too so we could forget about this problem forever.

So my lookup was still incomplete.

If not I will be happy to do it. However in this case we need someone
familiar with affected parts of the code who will be willing to re-check
a new patch since I'm not filling particularly confident about how
exactly errors should be handled in all these cases.

I'll do it and compress that in my patch.

By the way maybe someone knows other procedures besides malloc, realloc
and strdup that require special attention?

I don't know how you did it, but this email has visibly broken the
original thread. Did you change the topic name?

./src/backend/postmaster/postmaster.c: userDoption =
strdup(optarg);
[...]
./src/backend/bootstrap/bootstrap.c: userDoption =
strdup(optarg);
[...]
./src/backend/tcop/postgres.c: userDoption = strdup(optarg);
We cannot use pstrdup here because memory contexts are not set up
here, still it would be better than crashing, but I am not sure if
that's worth complicating this code.. Other opinions are welcome.

./contrib/vacuumlo/vacuumlo.c: param.pg_user = strdup(optarg);
[...]
./contrib/pg_standby/pg_standby.c: triggerPath = strdup(optarg);
[...]
./src/bin/pg_archivecleanup/pg_archivecleanup.c:
additional_ext = strdup(optarg); /* Extension to remove
Right we can do something here with pstrdup().

./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *)
malloc(sizeof(SPIPlanPtr));
Regarding refint.c, you can see upthread. Instead of reworking the
code it would be better to just drop it from the tree.

./src/backend/utils/adt/pg_locale.c: grouping = strdup(extlconv->grouping);
Here that would be a failure with an elog() as this is getting used by
things like NUM_TOCHAR_finish and PGLC_localeconv.

./src/backend/utils/mmgr/mcxt.c: node = (MemoryContext) malloc(needed);
You cannot do much here...

./src/timezone/zic.c: lcltime = strdup(optarg);
This is upstream code, not worth worrying.

./src/pl/tcl/pltcl.c: prodesc->user_proname =
strdup(NameStr(procStruct->proname));
./src/pl/tcl/pltcl.c: prodesc->internal_proname =
strdup(internal_proname);
./src/pl/tcl/pltcl.c- if (prodesc->user_proname == NULL ||
prodesc->internal_proname == NULL)
./src/pl/tcl/pltcl.c- ereport(ERROR,
./src/pl/tcl/pltcl.c- (errcode(ERRCODE_OUT_OF_MEMORY),
./src/pl/tcl/pltcl.c- errmsg("out of memory")));
Ah, yes. Here we need to do a free(prodesc) first.

./src/common/exec.c: putenv(strdup(env_path));
set_pglocale_pgservice() is used at the beginning of each process run,
meaning that a failure here would be printf(stderr), followed by
exit() for frontend, even ECPG as this compiles with -DFRONTEND.
Backend can use elog(ERROR) btw.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers