Thread: Lack of Sanity Checking in file 'pctcl.c' for PostgreSQL 9.4.x

On Fri, Jun 12, 2015 at 4:22 AM, Bill Parker wrote:
>    In reviewing some code, in directory 'postgresql-9.4.3/src/pl/tcl',
> file 'pltcl.c', there are several instances where calls to malloc()
> are made, but no check for a return value of NULL is made, which
> would indicate failure.   Additionally, it appears when malloc()
> returns NULL, previously allocated memory in function 'perm_fmgr_info'
> is not released, which could lead to memory leaks (even though the
> comment at the top says 'this routine is a crock' :)
>
> If you know how this problem might be fixed, list the solution below:
> Please feel free to review and comment on the above patch file...

Oh, nice catch again.

> I am attaching the patch file to this bug report

By the way, your patch does not compile properly and is not in-line
with the project's code format. See the updated patch attached ;)
--
Michael

Michael Paquier wrote:
> On Fri, Jun 12, 2015 at 4:22 AM, Bill Parker wrote:
> >    In reviewing some code, in directory 'postgresql-9.4.3/src/pl/tcl',
> > file 'pltcl.c', there are several instances where calls to malloc()
> > are made, but no check for a return value of NULL is made, which
> > would indicate failure.   Additionally, it appears when malloc()
> > returns NULL, previously allocated memory in function 'perm_fmgr_info'
> > is not released, which could lead to memory leaks (even though the
> > comment at the top says 'this routine is a crock' :)
> >
> > If you know how this problem might be fixed, list the solution below:
> > Please feel free to review and comment on the above patch file...
>
> Oh, nice catch again.
>
> > I am attaching the patch file to this bug report
>
> By the way, your patch does not compile properly and is not in-line
> with the project's code format. See the updated patch attached ;)

... or the conventions for allocating memory.  Why not just use palloc()?


--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Michael Paquier wrote:
>> By the way, your patch does not compile properly and is not in-line
>> with the project's code format. See the updated patch attached ;)

> ... or the conventions for allocating memory.  Why not just use palloc()?

That's hardly the fault of the proposed patch.  But yeah, it seems like
much the best fix here is to get rid of the malloc (and strdup) calls in
this code in favor of using the palloc infrastructure.  Even the calls
that *do* have manual failure checks are not compliant with our usual
coding standards.

            regards, tom lane

On Sat, Jun 13, 2015 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Michael Paquier wrote:
>>> By the way, your patch does not compile properly and is not in-line
>>> with the project's code format. See the updated patch attached ;)
>
>> ... or the conventions for allocating memory.  Why not just use palloc()?
>
> That's hardly the fault of the proposed patch.  But yeah, it seems like
> much the best fix here is to get rid of the malloc (and strdup) calls in
> this code in favor of using the palloc infrastructure.  Even the calls
> that *do* have manual failure checks are not compliant with our usual
> coding standards.

Hm. Regarding the code path mentioned by Bill something like the patch
attached is enough with a memory context for the query description.
Now, perhaps we could do more efforts with prodesc as well, see for
example compile_pltcl_function for pltcl and similarly for plperl.
Thoughts?
--
Michael

Alvaro Herrera wrote:

> > Now, perhaps we could do more efforts with prodesc as well, see for
> > example compile_pltcl_function for pltcl and similarly for plperl.
> > Thoughts?
>
> Right.  It would simplify the code: create a memory context child of
> TopTransactionContext, then compile the function, and if successful,
> then MemoryContextSetParent to some longer-lived context.  When the
> function is invalidated, it's sufficient to delete the context and
> create a new one.  Creating the context as child of
> TopTransactionContext allows you to avoid an explicit
> MemoryContextDelete() in the elog(ERROR) cases while compiling.

With some additional effort, we could get rid of perm_fmgr_info, at
least in pltcl.  (That hack was introduced in a3ed622b63b and
7748e9e7e5a back in 2001 and we never actually fixed it ...)

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier wrote:
> On Sat, Jun 13, 2015 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> >> Michael Paquier wrote:
> >>> By the way, your patch does not compile properly and is not in-line
> >>> with the project's code format. See the updated patch attached ;)
> >
> >> ... or the conventions for allocating memory.  Why not just use palloc()?
> >
> > That's hardly the fault of the proposed patch.  But yeah, it seems like
> > much the best fix here is to get rid of the malloc (and strdup) calls in
> > this code in favor of using the palloc infrastructure.  Even the calls
> > that *do* have manual failure checks are not compliant with our usual
> > coding standards.
>
> Hm. Regarding the code path mentioned by Bill something like the patch
> attached is enough with a memory context for the query description.

Right.  Note this no longer needs the individual pfree() when aborting,
because the MemoryContextDelete would remove the whole thing.  (Don't
bother resubmitting; I have already fixed it.)  Will push shortly.

> Now, perhaps we could do more efforts with prodesc as well, see for
> example compile_pltcl_function for pltcl and similarly for plperl.
> Thoughts?

Right.  It would simplify the code: create a memory context child of
TopTransactionContext, then compile the function, and if successful,
then MemoryContextSetParent to some longer-lived context.  When the
function is invalidated, it's sufficient to delete the context and
create a new one.  Creating the context as child of
TopTransactionContext allows you to avoid an explicit
MemoryContextDelete() in the elog(ERROR) cases while compiling.

I do wonder how many users this code has ...

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sun, Jul 19, 2015 at 5:19 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Alvaro Herrera wrote:
>
>> > Now, perhaps we could do more efforts with prodesc as well, see for
>> > example compile_pltcl_function for pltcl and similarly for plperl.
>> > Thoughts?
>>
>> Right.  It would simplify the code: create a memory context child of
>> TopTransactionContext, then compile the function, and if successful,
>> then MemoryContextSetParent to some longer-lived context.  When the
>> function is invalidated, it's sufficient to delete the context and
>> create a new one.  Creating the context as child of
>> TopTransactionContext allows you to avoid an explicit
>> MemoryContextDelete() in the elog(ERROR) cases while compiling.
>
> With some additional effort, we could get rid of perm_fmgr_info, at
> least in pltcl.  (That hack was introduced in a3ed622b63b and
> 7748e9e7e5a back in 2001 and we never actually fixed it ...)

Yes it seems so, even for plperl and plpython there seem to be some
room for improvement at quick glance... This looks like a master-only
change to me though (and I am sure we are on the same page). For
back-branches something like the previous patch is definitely safer.
--
Michael

Michael Paquier wrote:
> On Sun, Jul 19, 2015 at 5:19 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:

> > With some additional effort, we could get rid of perm_fmgr_info, at
> > least in pltcl.  (That hack was introduced in a3ed622b63b and
> > 7748e9e7e5a back in 2001 and we never actually fixed it ...)
>
> Yes it seems so, even for plperl and plpython there seem to be some
> room for improvement at quick glance... This looks like a master-only
> change to me though (and I am sure we are on the same page). For
> back-branches something like the previous patch is definitely safer.

Yes, agreed on both counts.  I will push the patch when I have a moment.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier wrote:

> > With some additional effort, we could get rid of perm_fmgr_info, at
> > least in pltcl.  (That hack was introduced in a3ed622b63b and
> > 7748e9e7e5a back in 2001 and we never actually fixed it ...)
>
> Yes it seems so, even for plperl and plpython there seem to be some
> room for improvement at quick glance... This looks like a master-only
> change to me though (and I am sure we are on the same page). For
> back-branches something like the previous patch is definitely safer.

Pushed.

All in all, the whole business of memory allocation in pltcl is pretty
nasty.  If people is really interested in pltcl, that should probably be
fixed to avoid memory leaks.  For one thing, the query cache hash is
global to the interpreter, not local to the function; whenever a
function is recompiled, the old SPI_prepared plans linger forever in the
interpreter (for the life of the session).  It would be nicer if those
were tied to the function lifetime.  For this, the hashtable would have
to be moved to the query desc struct.  pltcl_spi_prepare would have to
know the currently executing function desc struct ...

Maybe Bill is interested enough to write a patch?

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jul 20, 2015 at 9:29 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>
>> > With some additional effort, we could get rid of perm_fmgr_info, at
>> > least in pltcl.  (That hack was introduced in a3ed622b63b and
>> > 7748e9e7e5a back in 2001 and we never actually fixed it ...)
>>
>> Yes it seems so, even for plperl and plpython there seem to be some
>> room for improvement at quick glance... This looks like a master-only
>> change to me though (and I am sure we are on the same page). For
>> back-branches something like the previous patch is definitely safer.
>
> Pushed.
>
> All in all, the whole business of memory allocation in pltcl is pretty
> nasty.  If people is really interested in pltcl, that should probably be
> fixed to avoid memory leaks.  For one thing, the query cache hash is
> global to the interpreter, not local to the function; whenever a
> function is recompiled, the old SPI_prepared plans linger forever in the
> interpreter (for the life of the session).  It would be nicer if those
> were tied to the function lifetime.  For this, the hashtable would have
> to be moved to the query desc struct.  pltcl_spi_prepare would have to
> know the currently executing function desc struct ...

If this is done, the same can be directly applied to plperl. For
plpython, we are going to visibly need a memory context initialized at
some point and rely on it for allocations instead of TopMemoryContext
when doing the various allocations... Do you think it is worth it in
this case?

For what it's worth, I have been playing for a couple of minutes with
the code of pltcl and plperl to replace the remaining malloc calls by
a memory context and remove the calls to perm_func_ctx. This does not
fix the memory leaks of pltcl when functions are recompiled by this is
working somewhat nicely with plperl by relying on free_plperl_function
and check-world passes. So attached they are. This seem to improve a
bit things to me. Thoughts?
--
Michael