pgsql: Convert contrib/seg's bool-returning SQL functions to V1 call co

On Fri, Apr 22, 2016 at 03:54:48PM +0000, Tom Lane wrote:

Convert contrib/seg's bool-returning SQL functions to V1 call convention.

It appears that we can no longer get away with using V0 call convention
for bool-returning functions in newer versions of MSVC. The compiler
seems to generate code that doesn't clear the higher-order bits of the
result register, causing the bool result Datum to often read as "true"
when "false" was intended. This is not very surprising, since the
function thinks it's returning a bool-width result but fmgr_oldstyle
assumes that V0 functions return "char *"; what's surprising is that
that hack worked for so long on so many platforms.

Does this warrant a change to the "Section 2" comment of postgres.h,
explaining that its precautions no longer suffice everywhere?

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 2016-04-26 22:38:28 -0400, Noah Misch wrote:

On Fri, Apr 22, 2016 at 03:54:48PM +0000, Tom Lane wrote:

Convert contrib/seg's bool-returning SQL functions to V1 call convention.

It appears that we can no longer get away with using V0 call convention
for bool-returning functions in newer versions of MSVC. The compiler
seems to generate code that doesn't clear the higher-order bits of the
result register, causing the bool result Datum to often read as "true"
when "false" was intended. This is not very surprising, since the
function thinks it's returning a bool-width result but fmgr_oldstyle
assumes that V0 functions return "char *"; what's surprising is that
that hack worked for so long on so many platforms.

Does this warrant a change to the "Section 2" comment of postgres.h,
explaining that its precautions no longer suffice everywhere?

I don't understand why we don't just drop V0. It makes debugging harder,
exploitation easier (call arbitrary functions), and really has no
features making it desirable.

Andres

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Noah Misch <noah@leadboat.com> writes:

On Fri, Apr 22, 2016 at 03:54:48PM +0000, Tom Lane wrote:

Convert contrib/seg's bool-returning SQL functions to V1 call convention.

It appears that we can no longer get away with using V0 call convention
for bool-returning functions in newer versions of MSVC. The compiler
seems to generate code that doesn't clear the higher-order bits of the
result register, causing the bool result Datum to often read as "true"
when "false" was intended. This is not very surprising, since the
function thinks it's returning a bool-width result but fmgr_oldstyle
assumes that V0 functions return "char *"; what's surprising is that
that hack worked for so long on so many platforms.

Does this warrant a change to the "Section 2" comment of postgres.h,
explaining that its precautions no longer suffice everywhere?

Hmmm ... looking at that again, and particularly at the definition
of DatumGetBool, I wonder whether the true problem is that a cast to
bool is being interpreted differently than we expect. The intention,
as per its comment, is that we should consider only the rightmost byte
of the Datum value but consider any nonzero value of that byte as "true".
But if MSVC is now treating bool according to C99 rules, it might be
interpreting "(bool) (X)" as "(X) != 0".

It would be interesting to find out if VS2015 passes with this commit
reverted and instead defining DatumGetBool as, say,

#define DatumGetBool(X) ((bool) (GET_1_BYTE(X) != 0))

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Andres Freund <andres@anarazel.de> writes:

I don't understand why we don't just drop V0. It makes debugging harder,
exploitation easier (call arbitrary functions), and really has no
features making it desirable.

What's the argument that it makes debugging harder? Especially if
you aren't using it?

I don't particularly buy the "easier exploitation" argument, either.
You can't create a C function without superuser, and if you've got
superuser there are plenty of ways to run arbitrary code.

I'd agree that there are no desirable features that would motivate
writing new code in V0. But that's not the reason for keeping it;
the reason for keeping it is to avoid unnecessarily breaking
existing extension code.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 2016-04-26 22:59:44 -0400, Tom Lane wrote:

What's the argument that it makes debugging harder? Especially if
you aren't using it?

If you try to write a V1 function, but forget or mistype/rename the
function in PG_FUNCTION_INFO_V1, you'll get crashes, at least if you're
lucky.

I don't particularly buy the "easier exploitation" argument, either.
You can't create a C function without superuser, and if you've got
superuser there are plenty of ways to run arbitrary code.

Without pl*u installed, I don't think any of them are as simple as
calling system(). But yea, it's not a very high barrier.

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Wed, Apr 27, 2016 at 12:04 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-04-26 22:59:44 -0400, Tom Lane wrote:

What's the argument that it makes debugging harder? Especially if
you aren't using it?

If you try to write a V1 function, but forget or mistype/rename the
function in PG_FUNCTION_INFO_V1, you'll get crashes, at least if you're
lucky.

At some point we'll surely arrive at dropping it... Now if V0 is
decided to be dropped, making a deprecation notice in the release
notes of major version X and actually dropping it 2-3 years after
would be really welcome to ease the transaction. I am guessing that
you meant that.
--
Michael

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Tue, Apr 26, 2016 at 10:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

I don't understand why we don't just drop V0. It makes debugging harder,
exploitation easier (call arbitrary functions), and really has no
features making it desirable.

What's the argument that it makes debugging harder? Especially if
you aren't using it?

I don't particularly buy the "easier exploitation" argument, either.
You can't create a C function without superuser, and if you've got
superuser there are plenty of ways to run arbitrary code.

I'd agree that there are no desirable features that would motivate
writing new code in V0. But that's not the reason for keeping it;
the reason for keeping it is to avoid unnecessarily breaking
existing extension code.

I don't think that argument holds much water any more. The V1
interface was added in 0a7fb4e9184539afcb6fed0f1d2bc0abddc2b0a6, more
than 15 years ago. Anybody who has extension code that old that still
does anything useful and hasn't needed much bigger changes that
conversion to V1 calling convention deserves a medal. But more than
that, it's unreasonable to expect 15-year-plus deprecation windows for
features that are only exposed via C. If we stuck to that rigidly it
would be a major impediment to forward progress.

Let's add a GUC allow_oldstyle_functions with a default of off, and
make fetch_finfo_record() throw an ERROR in the infofunc == NULL case
unless allow_oldstyle_functions = on. That way, anybody who still
really wants to use V0 can do so. For everybody else, the very first
attempt to execute a function where you've forgotten or fat-fingered
the PG_FUNCTION_INFO_V1 decoration will produce a clear error message
telling you exactly what you did wrong. I confidently predict a lot
more people will be happy about that development than will be sad
about having to set a GUC to make their V0 functions work.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

Let's add a GUC allow_oldstyle_functions with a default of off, and
make fetch_finfo_record() throw an ERROR in the infofunc == NULL case
unless allow_oldstyle_functions = on.

This is not about "V0 calling convention is awful". It isn't, really,
unless you need to deal with nulls. This is about "allowing the finfo
record to be missing is error-prone". What about inventing a
PG_FUNCTION_INFO_V0() macro, and then defining the new GUC as "must
find a matching finfo record"? That would present less conversion
work for people who still have V0-style functions (and I'm sure there
are more than a few of them out there).

Now, if VS2015 actually has broken bool-returning V0, the argument for
keeping it going becomes a lot weaker. But at this point I suspect
strongly that that's not what happened but rather we've got a faulty
bool cast there, which if true is something we need to fix regardless
of any considerations about V0. Would someone please try the experiment
requested upthread?

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

On Wed, Apr 27, 2016 at 9:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Let's add a GUC allow_oldstyle_functions with a default of off, and
make fetch_finfo_record() throw an ERROR in the infofunc == NULL case
unless allow_oldstyle_functions = on.

This is not about "V0 calling convention is awful". It isn't, really,
unless you need to deal with nulls. This is about "allowing the finfo
record to be missing is error-prone". What about inventing a
PG_FUNCTION_INFO_V0() macro, and then defining the new GUC as "must
find a matching finfo record"? That would present less conversion
work for people who still have V0-style functions (and I'm sure there
are more than a few of them out there).

I'm not, but I don't have a problem with PG_FUNCTION_INFO_V0.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Apr 27, 2016 at 10:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Now, if VS2015 actually has broken bool-returning V0, the argument for
keeping it going becomes a lot weaker. But at this point I suspect
strongly that that's not what happened but rather we've got a faulty
bool cast there, which if true is something we need to fix regardless
of any considerations about V0. Would someone please try the experiment
requested upthread?

I just gave it a try. And by using PG_1_BYTE() the tests of
contrib/seg/ are passing.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:

On Wed, Apr 27, 2016 at 10:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Now, if VS2015 actually has broken bool-returning V0, the argument for
keeping it going becomes a lot weaker. But at this point I suspect
strongly that that's not what happened but rather we've got a faulty
bool cast there, which if true is something we need to fix regardless
of any considerations about V0. Would someone please try the experiment
requested upthread?

I just gave it a try. And by using PG_1_BYTE() the tests of
contrib/seg/ are passing.

Thanks! Barring objections, I will revert c8e81afc60093b19 tomorrow
and instead fix DatumGetBool.

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