Badly broken logic in plpython composite-type handling
I looked into the crash reported in bug #13579. The proximate cause
of the crash is that PLyString_ToComposite does this:
PLy_output_datum_func2(&info->out.d, typeTup,
exec_ctx->curr_proc->langid,
exec_ctx->curr_proc->trftypes);
without any thought for the possibility that info->is_rowtype is 1,
in which case it's stomping on the info->out.r data that will be
needed later. I have serious doubts that making PLyTypeOutput a
union was a wise idea, as it means that C offers you exactly zero
protection against this type of data clobber.
I tried changing that to
PLy_output_datum_func(info, typeTup,
exec_ctx->curr_proc->langid,
exec_ctx->curr_proc->trftypes);
which unsurprisingly results in "ERROR: PLyTypeInfo struct is initialized
for a Tuple" in the case reported in the bug ... and also results in quite
a few of the plpython regression tests failing that way, which makes it a
bit astonishing that we've not tripped over this bug already.
I'm inclined to think that maybe PLyString_ToComposite needs to be doing
something more like what PLyObject_ToComposite does, ie doing its own
lookup in a private descriptor; but I'm not sure if that's right, and
anyway it would just be doubling down on a bad design. Not being able
to cache these I/O function lookups is really horrid.
I wonder if that could be fixed by changing PLyTypeInput and PLyTypeOutput
from unions to structs and getting rid of is_rowtype in favor of allowing
the d and r fields to be valid or not independently; then you could treat
the object as either scalar or rowtype at need.
Does whoever designed this code in the first place want to fix it?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
I looked into the crash reported in bug #13579. The proximate cause
of the crash is that PLyString_ToComposite does this:
...
I'm inclined to think that maybe PLyString_ToComposite needs to be doing
something more like what PLyObject_ToComposite does, ie doing its own
lookup in a private descriptor; but I'm not sure if that's right, and
anyway it would just be doubling down on a bad design. Not being able
to cache these I/O function lookups is really horrid.
I wrote a draft patch that does it that way. It indeed stops the crash,
and even better, makes PLyString_ToComposite actually work for RECORD,
which AFAICT it's never done before. We'd conveniently omitted to test
the case in the plpython regression tests, thus masking the fact that
it would crash horribly if you invoked such a case more than once.
To make it work, I had to modify record_in to allow inputting a value
of type RECORD as long as it's given a correct typmod. While that would
not happen in ordinary SQL, it's possible in this and probably other
usages, and I can't really see any downside. The emitted record will
be properly marked with type RECORDOID and typmod whatever the internal
anonymous-type identifier is, and it's no more or less type-safe than
any other such value.
This version of PLyString_ToComposite is no better than
PLyObject_ToComposite as far as leaking memory goes. We could probably
fix that in a crude fashion by using plpython's "scratch context" for the
transient type-lookup data, but I'd rather see a proper fix wherein the
lookup results stay cached. In any case, that's a separate and less
critical matter.
Barring objections, I propose to commit and back-patch this. The crash
can be demonstrated back to 9.1.
regards, tom lane
Attachments:
plpython-returns-record-fix.patchtext/x-diff; charset=us-ascii; name=plpython-returns-record-fix.patchDownload+297-47
On 08/19/2015 05:05 PM, Tom Lane wrote:
Barring objections, I propose to commit and back-patch this. The crash
can be demonstrated back to 9.1.regards, tom lane
+1
--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers