Horton

Thread

Commits

  1. Don't crash if cursor_to_xmlschema is used on a non-data-returning Portal.

  1. function "cursor_to_xmlschema" causes a crash

    yangboyu <yangboyu.yby@alibaba-inc.com> — 2023-09-18T05:00:59Z

    Hello postgres hackers:
I recently notice that function "cursor_to_xmlschema" can lead to a crash if the
cursor parameter points to the query itself. Here is an example:

postgres=# SELECT cursor_to_xmlschema('' :: refcursor, TRUE , FALSE , 'xxx' ) into temp;
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.

The reason could be that this function doesn't ensure the cursor is correctly
opened, as a "select into" statement can't be opened as a cursor. Although it may
be challenging to perform a perfect check in this scenario, it seems sufficient
just to check the tuple descriptor of the portal, since only the query that
returns tuples can be opened as a cursor.

Only in my opinion, self-pointing cursors like this do not make practical sense.
This bug is discovered through randomly generated SQL statements.

Best regards,
Boyu Yang

  2. Re: function "cursor_to_xmlschema" causes a crash

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-09-18T17:01:42Z

    "=?UTF-8?B?5p2o5Lyv5a6HKOmVv+Wggik=?=" <yangboyu.yby@alibaba-inc.com> writes:
> I recently notice that function "cursor_to_xmlschema" can lead to a crash if the
> cursor parameter points to the query itself. Here is an example:

> postgres=# SELECT cursor_to_xmlschema('' :: refcursor, TRUE , FALSE , 'xxx' ) into temp;
> server closed the connection unexpectedly

Hmm, yeah, it's blindly assuming that whatever portal you point it at
must have a result tupdesc, which in general PORTAL_MULTI_QUERY
portals wouldn't.

I always ask myself "where else did we make the same mistake?".
Trawling through the callers of SPI_cursor_find and GetPortalByName,
I couldn't find any other places that might get a hard failure.
There are some where you might get weird error messages, eg

	regression=# select cursor_to_xml('',1,false,false,'');
	ERROR:  portal "" cannot be run

but maybe that's good enough.  The thing that is disturbing is that
SPI_cursor_find is a documented entry point, and the documentation
doesn't warn that what you get back might not be a cursor-like
object, so it seems like external callers might have this bug too.

I thought about having SPI_cursor_find refuse to return things that
aren't cursors, but I see that plpgsql's exec_stmt_open and
exec_stmt_forc use SPI_cursor_find just to check for a duplicate
cursor name.  There, it seems like we don't want any additional
filtering, because any portal will create a name conflict whether
it's a cursor or not.  While we could change those two callers to
use GetPortalByName instead, any such change would make things
strictly worse for outside callers that are doing the same thing.

Also, PerformPortalFetch and PerformPortalClose have this very
interesting kluge:

    /*
     * Disallow empty-string cursor name (conflicts with protocol-level
     * unnamed portal).
     */
    if (!stmt->portalname || stmt->portalname[0] == '\0')
        ereport(ERROR,
                (errcode(ERRCODE_INVALID_CURSOR_NAME),
                 errmsg("invalid cursor name: must not be empty")));

We could imagine making SPI_cursor_find do likewise, but again
I'm not sure if that'd be good for all callers.  In any case,
the unnamed portal isn't the only source of this sort of problem.
The argument for doing it would not be to protect careless callers
like cursor_to_xmlschema, but to prevent SPI clients from having
side-effects on the state of the unnamed portal.

On balance I'm satisfied with just changing cursor_to_xmlschema
as you suggest, though I'd probably phrase the error message
as "portal %s is not a cursor".  However, I feel like we'd better
extend the documentation for SPI_cursor_find to point out that
you might get something that isn't functionally like a cursor.

			regards, tom lane