Re: Missing checks when malloc returns NULL...

On Sat, Aug 27, 2016 at 12:33 AM, Aleksander Alekseev
<a(dot)alekseev(at)postgrespro(dot)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