Re: [HACKERS] Constifying numeric.c's local vars
Mark Dilger writes: >> 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. > I still believe this is unsafe. I'm with Andres: I don't see the problem. It's true that we've casted away a chance for the compiler to notice a problem, but that's not the only defense against problems. If we did try to modify const_zero, what should happen now is that we get a SIGSEGV from trying to scribble on read-only memory. But that's actually a step forward from before. Before, we'd have successfully modified the value of const_zero and thereby silently bollixed subsequent computations using it. Since, in fact, the code should never try to modify const_zero, the SIGSEGV should never happen. So effectively we have a hardware-enforced Assert that we don't modify it, and that seems good. As far as compiler-detectable mistakes go, Andres' changes to declare various function inputs as const seem like a pretty considerable improvement too, even if they aren't offering 100% coverage. regards, tom lane
Re: [HACKERS] Constifying numeric.c's local vars
> 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.
I still believe this is unsafe.
> 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.
This is probably safe, though, since NumericDigit seems to just be a typedef
to int16. I should have checked that definition before complaining about that
part.
Re: [HACKERS] Constifying numeric.c's local vars
On February 21, 2018 8:49:51 AM PST, Mark Dilger wrote: > >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. Just getting started for the day, no coffee yet. But, uh, what you're appear to be saying is that we have code modifying the digits of the zero value? That's seems unlikely. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [HACKERS] Constifying numeric.c's local vars
> On Sep 11, 2017, at 5:10 AM, Tom Lane wrote:
>
> Andres Freund 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
