Re: Missing checks when malloc returns NULL...

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>, a(dot)alekseev(at)postgrespro(dot)ru
Subject: Re: Missing checks when malloc returns NULL...
Date: 2016-08-29 07:42:41
Message-ID: CAB7nPqSCxBWt7Q1dpFTekg-zDx_GZ-ELgEOfYJ47zrD8=v=yHg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sat, Aug 27, 2016 at 10:24 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> ./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.

Those are not changed. We could have some NULL-checks but I am not
sure that's worth the complication here.

> ./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().

Those are updated with pg_strdup().

> ./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.

I'd rather see this nuked out of the surface of the code 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.

Done.

> ./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.

Done.

> ./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.

Done.

Regarding the handling of strdup in libpq the code is careful in its
handling after a review, so we had better do nothing.

After that, there are two calls to realloc and one call to malloc that
deserve some attention, though those happen in pgc.l and I am not
exactly sure how to handle them.

As Alexander's email
(https://www.postgresql.org/message-id/20160826153358.GA29981%40e733)
has broken this thread, I am attaching to this thread the analysis
report that has been generated by Alexander previously. It was
referenced in only an URL.

Attached is an updated patch addressing those extra points.
--
Michael

Attachment Content-Type Size
83287ef7d2_malloc.txt text/plain 16.5 KB
malloc-nulls-v4.patch application/x-patch 25.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-08-29 07:51:09 Re: pg_dump with tables created in schemas created by extensions
Previous Message Magnus Hagander 2016-08-29 07:04:39 Re: Renaming of pg_xlog and pg_clog