Обсуждение: undersized unions

Hi,

gcc warns about code like this:

typedef union foo
{
   int i;
   long long l;
} foo;

foo * assign(int i) {
    foo *p = (foo *) __builtin_malloc(sizeof(int));
    p->i = i;

    return p;
}


<source>: In function 'assign':
<source>:9:6: warning: array subscript 'foo[0]' is partly outside array bounds of 'unsigned char[4]' [-Warray-bounds=]
    9 |     p->i = i;
      |      ^~
<source>:8:22: note: object of size 4 allocated by '__builtin_malloc'
    8 |     foo *p = (foo *) __builtin_malloc(sizeof(int));
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Compiler returned: 0



I can't really tell if gcc is right or wrong wrong to warn about
this. On the one hand it's a union, and we only access the element that
is actually backed by memory, on the other hand, the standard does say
that the size of a union is the largest element, so we are pointing to
something undersized.


We actually have a fair amount of code like that, but currently are
escaping most of the warnings, because gcc doesn't know that palloc() is
an allocator. With more optimizations (particularly with LTO), we end up
with more of such warnings.  I'd like to annotate palloc so gcc
understands the size, as that does help to catch bugs when confusing the
type. It also helps static analyzers.


An example of such code in postgres:

../../home/andres/src/postgresql/src/backend/utils/adt/numeric.c: In function 'make_result_opt_error':
../../home/andres/src/postgresql/src/backend/utils/adt/numeric.c:7628:23: warning: array subscript 'struct
NumericData[0]'is partly outside array bounds of 'unsigned char[6]' [-Warray-bounds=]
 
 7628 |                 result->choice.n_header = sign;
      |                       ^~
../../home/andres/src/postgresql/src/backend/utils/adt/numeric.c:7625:36: note: object of size 6 allocated by 'palloc'
 7625 |                 result = (Numeric) palloc(NUMERIC_HDRSZ_SHORT);
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~

Given that Numeric is defined as:

struct NumericData
{
    int32        vl_len_;        /* varlena header (do not touch directly!) */
    union NumericChoice choice; /* choice of format */
};

and
#define NUMERIC_HDRSZ_SHORT (VARHDRSZ + sizeof(uint16))

Here I can blame gcc even less - result is indeed not a valid pointer to
struct NumericData, because sizeof(NumericData) is 8, not 6.  I suspect
it's actually undefined behaviour to ever dereference a Numeric pointer,
when the pointer points to something smaller than sizeof(NumericData).


Greetings,

Andres Freund

On Sat, Feb 04, 2023 at 05:07:08AM -0800, Andres Freund wrote:
> <source>: In function 'assign':
> <source>:9:6: warning: array subscript 'foo[0]' is partly outside array bounds of 'unsigned char[4]'
[-Warray-bounds=]
>     9 |     p->i = i;
>       |      ^~
> <source>:8:22: note: object of size 4 allocated by '__builtin_malloc'
>     8 |     foo *p = (foo *) __builtin_malloc(sizeof(int));
>       |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Compiler returned: 0
>
> I can't really tell if gcc is right or wrong wrong to warn about
> this. On the one hand it's a union, and we only access the element that
> is actually backed by memory, on the other hand, the standard does say
> that the size of a union is the largest element, so we are pointing to
> something undersized.

Something I have noticed, related to that..  meson reports a set of
warnings here, not ./configure, still I apply the same set of CFLAGS
to both.  What's the difference in the meson setup that creates that,
if I may ask?  There is a link to the way -Warray-bound is handled?

> We actually have a fair amount of code like that, but currently are
> escaping most of the warnings, because gcc doesn't know that palloc() is
> an allocator. With more optimizations (particularly with LTO), we end up
> with more of such warnings.  I'd like to annotate palloc so gcc
> understands the size, as that does help to catch bugs when confusing the
> type. It also helps static analyzers.

Ah, that seems like a good idea in the long run.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sat, Feb 04, 2023 at 05:07:08AM -0800, Andres Freund wrote:
>> We actually have a fair amount of code like that, but currently are
>> escaping most of the warnings, because gcc doesn't know that palloc() is
>> an allocator. With more optimizations (particularly with LTO), we end up
>> with more of such warnings.  I'd like to annotate palloc so gcc
>> understands the size, as that does help to catch bugs when confusing the
>> type. It also helps static analyzers.

> Ah, that seems like a good idea in the long run.

I'm kind of skeptical about whether we'll be able to get rid of all
the resulting warnings without extremely invasive (and ugly) changes.

            regards, tom lane

Hi,

On February 5, 2023 6:16:55 AM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Michael Paquier <michael@paquier.xyz> writes:
>> On Sat, Feb 04, 2023 at 05:07:08AM -0800, Andres Freund wrote:
>>> We actually have a fair amount of code like that, but currently are
>>> escaping most of the warnings, because gcc doesn't know that palloc() is
>>> an allocator. With more optimizations (particularly with LTO), we end up
>>> with more of such warnings.  I'd like to annotate palloc so gcc
>>> understands the size, as that does help to catch bugs when confusing the
>>> type. It also helps static analyzers.
>
>> Ah, that seems like a good idea in the long run.
>
>I'm kind of skeptical about whether we'll be able to get rid of all
>the resulting warnings without extremely invasive (and ugly) changes.

It's not that many sources of warnings, fwiw.

But the concrete reason for posting here was that I'm wondering whether the "undersized" allocations could cause
problemsas-is.  

On the one hand there's compiler optimizations that could end up being a problem - imagine two branches of an if
allocatingsomething containing a union and one assigning to 32 the other to a 64bit integer union member. It'd imo be
reasonablefor the compiler to move that register->memory move outside of the if. 

On the other hand, it also just seems risky from a code writing perspective. It's not immediate obvious that it'd be
unsafeto create an on-stack Numeric by assigning *ptr. But it is. 

Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Hi,

On 2023-02-05 10:18:14 +0900, Michael Paquier wrote:
> On Sat, Feb 04, 2023 at 05:07:08AM -0800, Andres Freund wrote:
> > <source>: In function 'assign':
> > <source>:9:6: warning: array subscript 'foo[0]' is partly outside array bounds of 'unsigned char[4]'
[-Warray-bounds=]
> >     9 |     p->i = i;
> >       |      ^~
> > <source>:8:22: note: object of size 4 allocated by '__builtin_malloc'
> >     8 |     foo *p = (foo *) __builtin_malloc(sizeof(int));
> >       |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > Compiler returned: 0
> >
> > I can't really tell if gcc is right or wrong wrong to warn about
> > this. On the one hand it's a union, and we only access the element that
> > is actually backed by memory, on the other hand, the standard does say
> > that the size of a union is the largest element, so we are pointing to
> > something undersized.
>
> Something I have noticed, related to that..  meson reports a set of
> warnings here, not ./configure, still I apply the same set of CFLAGS
> to both.  What's the difference in the meson setup that creates that,
> if I may ask?  There is a link to the way -Warray-bound is handled?

It's possibly related to the optimization level used. Need a bit more
information to provide a more educated guess. What warnings, what CFLAGS
etc.

Greetings,

Andres Freund

On Sun, Feb 5, 2023 at 6:28 AM Andres Freund <andres@anarazel.de> wrote:
> On the other hand, it also just seems risky from a code writing perspective. It's not immediate obvious that it'd be
unsafeto create an on-stack Numeric by assigning *ptr. But it is.
 

Well, I think that is pretty obvious: we have lots of things that are
essentially variable-length types, and you can't put any of them on
the stack.

But I do also think that the Numeric situation is messier than some
others we have got, and that's partly my fault, and it would be nice
to make it better.

I do not really know exactly how to do that, though. Our usual pattern
is to just have a struct and end with a variable-length array, or
alternatively add a comment says "other stuff follows!" at the end of
the struct definition, without doing anything that C knows about at
all. But here it's more complicated: there's a uint16 value for sure,
and then maybe an int16 value, and then some number of NumericDigit
values. That "maybe an int16 value" part is not something that C has a
built-in way of representing, to my knowledge, which is why we end up
with this hackish thing.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> I do not really know exactly how to do that, though. Our usual pattern
> is to just have a struct and end with a variable-length array, or
> alternatively add a comment says "other stuff follows!" at the end of
> the struct definition, without doing anything that C knows about at
> all. But here it's more complicated: there's a uint16 value for sure,
> and then maybe an int16 value, and then some number of NumericDigit
> values. That "maybe an int16 value" part is not something that C has a
> built-in way of representing, to my knowledge, which is why we end up
> with this hackish thing.

If we were willing to blow off the optimizations for NBASE < 10000,
and say that NumericDigit is always int16, then it would be possible
to represent all of these variants as plain array-of-int16, with
some conventions about which indexes are what (and some casting
between int16 and uint16).

I am, however, very dubious that Andres is correct that there's a
problem here.  Given that two of the variants of union NumericChoice
are structs ending with a flexible array, any compiler that thinks
it knows the size of the union precisely is broken.

            regards, tom lane

Hi

On 2023-02-06 11:42:57 -0500, Robert Haas wrote:
> On Sun, Feb 5, 2023 at 6:28 AM Andres Freund <andres@anarazel.de> wrote:
> > On the other hand, it also just seems risky from a code writing perspective. It's not immediate obvious that it'd
beunsafe to create an on-stack Numeric by assigning *ptr. But it is.
 
>
> Well, I think that is pretty obvious: we have lots of things that are
> essentially variable-length types, and you can't put any of them on
> the stack.

There's a difference between a stack copy not containing all the data, and a
stack copy triggering undefined behaviour. But I agree, it's not something
that'll commonly endanger us.


> But I do also think that the Numeric situation is messier than some
> others we have got, and that's partly my fault, and it would be nice
> to make it better.
>
> I do not really know exactly how to do that, though. Our usual pattern
> is to just have a struct and end with a variable-length array, or
> alternatively add a comment says "other stuff follows!" at the end of
> the struct definition, without doing anything that C knows about at
> all. But here it's more complicated: there's a uint16 value for sure,
> and then maybe an int16 value, and then some number of NumericDigit
> values. That "maybe an int16 value" part is not something that C has a
> built-in way of representing, to my knowledge, which is why we end up
> with this hackish thing.

Perhaps something like

typedef struct NumericBase
{
    uint16        n_header;
} NumericBase;

typedef struct NumericData
{
    int32        vl_len_;        /* varlena header (do not touch directly!) */
    NumericBase data;
} NumericData;

/* subclass of NumericBase, needs to start in a compatible way */
typedef struct NumericLong
{
    uint16        n_sign_dscale;    /* Sign + display scale */
    int16        n_weight;        /* Weight of 1st digit    */
} NumericLong;

/* subclass of NumericBase, needs to start in a compatible way */
struct NumericShort
{
    uint16        n_header;        /* Sign + display scale + weight */
    NumericDigit n_data[FLEXIBLE_ARRAY_MEMBER]; /* Digits */
};

Macros that e.g. access n_long would need to cast, before they're able to
access n_long. So we'd end up with something like

#define NUMERIC_SHORT(n)  ((NumericShort *)&((n)->data))
#define NUMERIC_LONG(n)  ((NumericLong *)&((n)->data))

#define NUMERIC_WEIGHT(n)    (NUMERIC_HEADER_IS_SHORT((n)) ? \
    ((NUMERIC_SHORT(n)->n_header & NUMERIC_SHORT_WEIGHT_SIGN_MASK ? \
        ~NUMERIC_SHORT_WEIGHT_MASK : 0) \
     | (NUMERIC_SHORT(n)->n_header & NUMERIC_SHORT_WEIGHT_MASK)) \
    : (NUMERIC_LONG(n)->n_weight))

Although I'd actually be tempted to rip out all the casts but NUMERIC_LONG()
in this case, because it's all all just accessing the base n_header anyway.

Greetings,

Andres Freund

Hi,

On 2023-02-06 11:55:40 -0500, Tom Lane wrote:
> I am, however, very dubious that Andres is correct that there's a
> problem here.  Given that two of the variants of union NumericChoice
> are structs ending with a flexible array, any compiler that thinks
> it knows the size of the union precisely is broken.

The compiler just complains about the minimum size of the union, which is
  Max(offsetof(NumericShort, n_data), offsetof(NumericLong, n_data))
IOW, our trickery with flexible arrays would allow us to allocate just 8 bytes
for a NumericData, but not just 6.

Flexible arrays allow the compiler to understand the variable size, but we
don't use it for all variability. Hence the warnings.

Greetings,

Andres Freund

On Mon, Feb 6, 2023 at 1:28 PM Andres Freund <andres@anarazel.de> wrote:
> Perhaps something like

Yeah, that'd work. You'd want a big ol' warning comment here:

> typedef struct NumericData
> {
>         int32           vl_len_;                /* varlena header (do not touch directly!) */
>         NumericBase data;
> } NumericData;

like /* actually NumericShort or NumericLong */ or something

> Although I'd actually be tempted to rip out all the casts but NUMERIC_LONG()
> in this case, because it's all all just accessing the base n_header anyway.

Yeah.

-- 
Robert Haas
EDB: http://www.enterprisedb.com