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