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

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.

The only functions of this description in core+contrib are in contrib/seg,
which we'd intentionally left mostly in V0 style to serve as a warning
canary if V0 call convention breaks.  We could imagine hacking things
so that they're still V0 (we'd have to redeclare the bool-returning
functions as returning some suitably wide integer type, like size_t,
at the C level).  But on the whole it seems better to convert 'em to V1.
We can still leave the pointer- and int-returning functions in V0 style,
so that the test coverage isn't gone entirely.

Back-patch to 9.5, since our intention is to support VS2015 in 9.5
and later.  There's no SQL-level change in the functions' behavior
so back-patching should be safe enough.

Discussion: <22094.1461273324@sss.pgh.pa.us>

Michael Paquier, adjusted some by me

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/c8e81afc60093b199a128ccdfbb692ced8e0c9cd

Modified Files
--------------
contrib/seg/seg.c | 300 +++++++++++++++++++++++++++++++++++-------------------
1 file changed, 195 insertions(+), 105 deletions(-)

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?

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

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

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

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.

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