Re: Missing checks when malloc returns NULL... - Mailing list pgsql-hackers

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