Re: BUG #19480: PL/Python SRF crashes (SIGSEGV) when function is replaced mid-iteration: use-after-free in PLy_funct

"Matheus Alcantara" <matheusssilv97@gmail.com> writes:
> Patch attached.

I had been planning to wait for v20 development to open, but with
RMT approval the target is now v19 instead, so I'd like to get
this done before the end of June.  I looked through the patch
and found a couple of issues immediately:

* Your refactoring to have just one PLy_procedure_get call in
plpython3_call_handler is no good.  You missed the comment
block just above:

    /*
     * Push execution context onto stack.  It is important that this get
     * popped again, so avoid putting anything that could throw error between
     * here and the PG_TRY.
     */
    exec_ctx = PLy_push_execution_context(!nonatomic);

+   proc = PLy_procedure_get(fcinfo, false);
+
    PG_TRY();
    {

I counsel putting those PLy_procedure_get calls back where they were.

* I also question the decision to refactor where/how is_trigger is
computed; that doesn't seem necessary to the purposes of the patch,
nor is it a clear improvement.  I'd just as soon leave that
mechanism alone as much as we can.  If there is an improvement to
be had, let's address that separately.  (Alternative thought:
should we rely on the isTrigger/isEventTrigger bools that
funccache.c sets up for us?  I'm not quite sure if getting friendly
with struct CachedFunctionHashKey is a good idea or not.)

* I find it confusing that you called "PLyProcedureCache *" variables
"pcache" in some places and "proc" in others.  The latter choice seems
poor because mostly "proc" is a PLyProcedure pointer.  Using "proc"
leads to constructions like "proc->proc", which I don't find
intelligible.

* The new code could do with more comments.  I realize that plpython
is poorly commented in many places, but let's see if we can leave it
better than we found it.

			regards, tom lane