Re: [HACKERS] Constifying numeric.c's local vars

> On Sep 11, 2017, at 5:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andres Freund <andres@anarazel.de> writes:
>> One large user of unnecessary non-constant static variables is
>> numeric.c.  More out of curiosity - numeric is slow enough in itself to
>> make inlining not a huge win - I converted it to use consts.
>
> LGTM.
>
>> It's a bit ugly that some consts have to be casted away in the constant
>> definitions, but aside from just inlining the values, I don't quite see
>> a better solution?
>
> No, I don't either.  I'm not sure that writing the constant inline would
> produce the desired results - the compiler might well decide that it had
> to be in read-write storage.
>
>             regards, tom lane

This patch got committed as c1898c3e1e235ae35b4759d233253eff221b976a
on Sun Sep 10 16:20:41 2017 -0700, but I've only just gotten around to
reviewing it.

I believe this is wrong and should be reverted, at least in part.

The NumericVar struct has the field 'digits' as non-const:

typedef struct NumericVar
{
    int         ndigits;        /* # of digits in digits[] - can be 0! */
    int         weight;         /* weight of first digit */
    int         sign;           /* NUMERIC_POS, NUMERIC_NEG, or NUMERIC_NAN */
    int         dscale;         /* display scale */
    NumericDigit *buf;          /* start of palloc'd space for digits[] */
    NumericDigit *digits;       /* base-NBASE digits */
} NumericVar;

The static const data which is getting put in read only memory sets that data
by casting away const as follows:

static const NumericDigit const_zero_data[1] = {0};
static const NumericVar const_zero =
{0, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_zero_data};

This means that the const variable 'const_zero' contains a pointer that is
non-const, pointing at something that is static const, stored in read only
memory.  Yikes.

The function set_var_from_var(const NumericVar *value, NumericVar *dest)
uses memcpy to copy the contents of value into dest.  In cases where the value
is a static const variable (eg, const_zero), the memcpy is copying a pointer to
static const read only data into the dest and implicitly casting away const.
Since that static const data is stored in read only memory, this has undefined
semantics, and I believe could lead to a server crash, at least on some
architectures with some compilers.

The idea that set_var_from_var might be called on const_zero (or const_one,
etc.) is not hypothetical.  It is being done in numeric.c.

If this is safe, somebody needs to be a lot clearer about why that is so.  There
are no comments explaining it in the file, and the conversation in this thread
never got into any details about it either.

Mark Dilger