Re: Error check always bypassed in tablefunc.c

Michael Paquier <michael.paquier@gmail.com> writes:
> So, I have been poking at this code a bit more and as the values of
> the parameters are passed as-is to the SQL queries that connectby
> generates internally (this is as well mentioned in the documentation
> here: http://www.postgresql.org/docs/devel/static/tablefunc.html), you
> can do quite fancy things by passing for example values of the type
> "foo FROM table; --" or similar. Particularly, by enforcing a query
> returning only one column, or NULL values I am even able to crash the
> server. The interesting part is that even if compatConnectbyTupleDescs
> is enabled for each level, it is still possible to crash the server by
> passing for example NULL values casted to the same type, like that
> 'NULL::text, NULL::text; --'.
> The attached patch fixes all those things, I have also enabled
> compatConnectbyTupleDescs to run at each level. I'll add it to the
> next CF as well to not lose track of it. This behavior has been like
> that forever...

Since this is a potential-core-dump fix, I went ahead and dealt with it
rather than waiting for the next CF.  I made a few adjustments:

* I thought that the way you changed the type compatibility tests was
overcomplicated and unnecessary.  As the code stands, there's no great
need to insist on type compatibility at all: if the printed form of
the constructed query's results is acceptable to the outer query's types,
it'll work, and otherwise will throw a reasonably intelligible error.
Now, we might well want to improve this code to skip the conversion to
text and back at some point, in which case we would need to insist on
a type match.  But in neither of those cases does it seem helpful to
ask whether there is a SQL type cast, as your patch was doing.  The
existence of a cast does not imply I/O representation compatibility,
so it's not in line with the current behavior, and if we want to skip
text conversion we'd prefer it's exactly the same type, which is the
check as it originally existed before it accidentally got broken.

What I did about this was to leave the behavior alone in back branches,
but insist on a type match in HEAD.  I think we can reasonably tighten
the type requirements in a new major release, but doing it in minor
releases is probably a bad idea.

* I thought it was odd to throw an error for NULL input, especially
an "infinite recursion" error.  However, even with your patch the code
would've dumped core on a null current_key value (since it would've
passed a null start_with down to the next recursion level).  What I
changed it to was to omit the recursion test (and hence print the row)
and then not recurse at a null.  This seems consistent and reasonable.

* I made a few other minor cleanups as well, in particular getting
rid of some unnecessary pstrdup() steps.
        regards, tom lane