Horton

Thread

Commits

  1. Run pgindent on the previous commit.

  2. Fix edge-case resource leaks in PL/Python error reporting.

  1. Confused coding in PLy_traceback()

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-31T20:33:56Z

    My attention happened to be drawn to plpython's PLy_traceback()
function, and I noted a couple of things that sure seem like bugs.
First, there's this bit:

	e_type_o = PyObject_GetAttrString(e, "__name__");
	e_module_o = PyObject_GetAttrString(e, "__module__");
	if (e_type_o)
		e_type_s = PyString_AsString(e_type_o);
	if (e_type_s)
		e_module_s = PyString_AsString(e_module_o);

Surely that second "if" is meant to be "if (e_module_o)"?  It doesn't
make any sense to be testing whether we could get a string from
e_type_o to decide if it's safe to touch e_module_o.  This is probably
only a latent bug because not getting these strings is a can't-happen
case, but still.

Second, the whole function shows truly remarkable faith that none of
what it calls will ever throw an error.  If that does happen, the code
will leak PyObject references --- and it *can* happen, if only because
of the possibility of OOM in string allocation.  Since the code seems
to be trying not to leak those, the fact that its coverage is so
incomplete seems like a bug.

I then realized that there's another fundamental risk of leaking
PyObject references, which is that this function is charged with
dropping all the references in the passed-in traceback stack;
but if it errors out anywhere, it'll fail to do that.  It would
be better for that responsibility to lie with PLy_elog_impl which
obtained the stack in the first place (and which also has unwarranted
faith in its callees not failing).

So I propose the attached.  For ease of review, I've not re-indented
the code that needs to move inside PG_TRY blocks.  Also, I dropped the
logic about pfree'ing the string buffers in PLy_elog_impl's PG_FINALLY
block: that doesn't seem necessary, and continuing to do it would
require making those things volatile which is notationally messy.

			regards, tom lane

  2. Re: Confused coding in PLy_traceback()

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-01T19:05:37Z

    I wrote:
> So I propose the attached.  For ease of review, I've not re-indented
> the code that needs to move inside PG_TRY blocks.  Also, I dropped the
> logic about pfree'ing the string buffers in PLy_elog_impl's PG_FINALLY
> block: that doesn't seem necessary, and continuing to do it would
> require making those things volatile which is notationally messy.

Pushed.  After sleeping on it I decided that removing the pfree's
would be a poor tradeoff, as it's not clear how long those allocations
might survive otherwise.  The extra "volatile" markers for "xmsg"
and "tbmsg" aren't that big a deal.  Trying to mark "emsg" as volatile
would be problematic because none of the StringInfo routines are set
up to allow that, but it shouldn't be a problem because that struct
will surely be on the stack.

			regards, tom lane