Re: [PATCH] testsuite: Handle default_packed targets in gcc.dg/plugin

[David Malcolm <dmalcolm@redhat.com>]

On Thu, 2023-03-16 at 19:25 +0100, Hans-Peter Nilsson wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Date: Thu, 16 Mar 2023 13:55:48 -0400
> 
> > On Thu, 2023-03-09 at 19:56 +0100, Hans-Peter Nilsson wrote:
> > > It's not obvious to me whether considered best to include or
> > > exclude these tests that depend on structure layout details.
> > > If excluding, the obvious alternative to this patch is then
> > > to add a top one-liner (to dg-skip-if the test for
> > > default_packed targets or a similar excluding expression).
> > > I'm fine either way, just suggesting the following, which
> > > handles the cris-elf test-case failures I see for these
> > > tests, and causes no change in results for native
> > > x86_64-pc-linux-gnu.
> > 
> > Thanks for looking at this.
> > 
> > How about a third option: can the structs be explicitly marked as
> > being
> > packed, by adding __attribute__((__packed__)) to the various
> > structs? 
> > The tests are all about detecting problems with padding bits, and
> > presumably we can have padding bits on all targets if we explicitly
> > ask
> > for them.
> > 
> > Does that make for a simpler patch?
> 
> Did I get you right: making the layout the same for all
> targets, by -for all structs that in my patch needed
> different layout- marking them with
> __attribute__((__packed__)) and adjust numbers in warnings?
> 
> That doesn't seem like a good idea.  At a glance the
> *testcode* will be simpler, but the patch will be slightly
> larger and have a lot of "-" lines instead of "+" lines, as
> the patch cause a lot of warnings to be dropped: you'll test
> for absence of warnings instead of proper warnings.
> 
> Looks like you'll lose 24 of the padding tests; 30 lines
> where I added "target ! default_packed" and 6 where I added
> "target default_packed".
> 
> Perhaps I misunderstood?

No, I think I'm misunderstanding the problem; sorry.

I think I prefer the top one-liner dg-skip-if approach you mentioned in
your original email; it seems simplest.

Thanks
Dave




> 
> brgds, H-P
> 
> 
> > Dave
> > 
> > > 
> > > Beware that some of the tests have lines with trailing
> > > whitespace.  Where lines are changed in this patch, the
> > > trailing whitespace is removed.
> > 
> > 
> > > 
> > > Ok to commit?
> > > 
> > > -- >8 --
> > > It's a judgement call whether to just skip some of these
> > > tests rather than trying to match messages depending on the
> > > layout of structures, but better include than exclude.
> > > 
> > >         * gcc.dg/plugin/infoleak-2.c,
> > >         gcc.dg/plugin/infoleak-CVE-2011-1078-1.c,
> > >         gcc.dg/plugin/infoleak-CVE-2011-1078-2.c,
> > >         gcc.dg/plugin/infoleak-CVE-2017-18549-1.c,
> > >         gcc.dg/plugin/infoleak-CVE-2017-18550-1.c,
> > >         gcc.dg/plugin/infoleak-antipatterns-1.c,
> > >         gcc.dg/plugin/infoleak-fixit-1.c: Handle default_packed
> > > targets.
> > > ---
> > >  gcc/testsuite/gcc.dg/plugin/infoleak-2.c            | 13
> > > ++++++++---
> > > --
> > >  .../gcc.dg/plugin/infoleak-CVE-2011-1078-1.c        | 10 +++++--
> > > ---
> > >  .../gcc.dg/plugin/infoleak-CVE-2011-1078-2.c        | 10 +++++--
> > > ---
> > >  .../gcc.dg/plugin/infoleak-CVE-2017-18549-1.c       | 10 +++++--
> > > ---
> > >  .../gcc.dg/plugin/infoleak-CVE-2017-18550-1.c       |  7 ++++---
> > >  .../gcc.dg/plugin/infoleak-antipatterns-1.c         | 10 +++++--
> > > ---
> > >  gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c      | 10 ++++++-
> > > ---
> > >  7 files changed, 38 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-2.c
> > > b/gcc/testsuite/gcc.dg/plugin/infoleak-2.c
> > > index 252f8f25918a..4ba484b3c6be 100644
> > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-2.c
> > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-2.c
> > > @@ -18,16 +18,19 @@ struct st
> > >    int b:1; /* { dg-message "field 'b' is uninitialized \\(1
> > > bit\\)"
> > > "field" } */
> > >             /* { dg-message "padding after field 'b' is
> > > uninitialized
> > > \\(7 bits\\)" "padding" { target *-*-* } .-1 } */
> > >    u8 d;    /* { dg-message "field 'd' is uninitialized \\(1
> > > byte\\)"
> > > } */
> > > -  int c:7; /* { dg-message "padding after field 'c' is
> > > uninitialized
> > > \\(9 bits\\)" } */
> > > -  u16 e;   /* { dg-message "padding after field 'e' is
> > > uninitialized
> > > \\(2 bytes\\)" } */  
> > > +  int c:7; /* { dg-message "padding after field 'c' is
> > > uninitialized
> > > \\(9 bits\\)" "padding" { target { ! default_packed } } } */
> > > +           /* { dg-message "padding after field 'c' is
> > > uninitialized
> > > \\(1 bit\\)" "padding" { target default_packed } .-1 } */
> > > +  u16 e;   /* { dg-message "padding after field 'e' is
> > > uninitialized
> > > \\(2 bytes\\)" "padding" { target { ! default_packed } } } */
> > >  };
> > >  
> > >  void test (void __user *dst, u16 v)
> > >  {
> > >    struct st s; /* { dg-message "region created on stack here"
> > > "where" } */
> > > -  /* { dg-message "capacity: 12 bytes" "capacity" { target *-*-*
> > > }
> > > .-1 } */
> > > -  /* { dg-message "suggest forcing zero-initialization by
> > > providing
> > > a '\\{0\\}' initializer" "fix-it" { target *-*-* } .-2 } */  
> > > +  /* { dg-message "capacity: 12 bytes" "capacity" { target { !
> > > default_packed } } .-1 } */
> > > +  /* { dg-message "capacity: 9 bytes" "capacity" { target
> > > default_packed } .-2 } */
> > > +  /* { dg-message "suggest forcing zero-initialization by
> > > providing
> > > a '\\{0\\}' initializer" "fix-it" { target *-*-* } .-3 } */
> > >    s.e = v;
> > >    copy_to_user(dst, &s, sizeof (struct st)); /* { dg-warning
> > > "potential exposure of sensitive information by copying
> > > uninitialized
> > > data from stack" "warning" } */
> > > -  /* { dg-message "10 bytes are uninitialized" "note how much" {
> > > target *-*-* } .-1 } */
> > > +  /* { dg-message "10 bytes are uninitialized" "note how much" {
> > > target { ! default_packed } } .-1 } */
> > > +  /* { dg-message "7 bytes are uninitialized" "note how much" {
> > > target default_packed } .-2 } */
> > >  }
> > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-
> > > 1.c
> > > b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-1.c
> > > index 3616fbe176b3..9269b911b22f 100644
> > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-1.c
> > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-1.c
> > > @@ -51,7 +51,7 @@ struct socket {
> > >  
> > >  struct sco_conninfo {
> > >         __u16 hci_handle;
> > > -       __u8  dev_class[3]; /* { dg-message "padding after field
> > > 'dev_class' is uninitialized \\(1 byte\\)" } */
> > > +       __u8  dev_class[3]; /* { dg-message "padding after field
> > > 'dev_class' is uninitialized \\(1 byte\\)" "padding" { target { !
> > > default_packed } } } */
> > >  };
> > >  
> > >  struct sco_conn {
> > > @@ -83,8 +83,8 @@ static int
> > > sco_sock_getsockopt_old_broken(struct
> > > socket *sock, int optname, char
> > >  {
> > >         struct sock *sk = sock->sk;
> > >         /* [...snip...] */
> > > -       struct sco_conninfo cinfo; /* { dg-message "region
> > > created on
> > > stack here" "where" } */
> > > -                                  /* { dg-message "capacity: 6
> > > bytes" "capacity" { target *-*-* } .-1 } */
> > > +       struct sco_conninfo cinfo; /* { dg-message "region
> > > created on
> > > stack here" "where" { target { ! default_packed } } } */
> > > +                                  /* { dg-message "capacity: 6
> > > bytes" "capacity" { target { ! default_packed } } .-1 } */
> > >         /* Note: 40 bits of fields, padded to 48.  */
> > >  
> > >         int len, err = 0;
> > > @@ -101,8 +101,8 @@ static int
> > > sco_sock_getsockopt_old_broken(struct
> > > socket *sock, int optname, char
> > >                 memcpy(cinfo.dev_class, sco_pi(sk)->conn->hcon-
> > > > dev_class, 3);
> > >  
> > >                 len = min_t(unsigned int, len, sizeof(cinfo));
> > > -               if (copy_to_user(optval, (char *)&cinfo, len)) /*
> > > {
> > > dg-warning "potential exposure of sensitive information by
> > > copying
> > > uninitialized data from stack" "warning" { target *-*-* } } */
> > > -                       /* { dg-message "1 byte is uninitialized"
> > > "how much note" { target *-*-* } .-1 } */
> > > +               if (copy_to_user(optval, (char *)&cinfo, len)) /*
> > > {
> > > dg-warning "potential exposure of sensitive information by
> > > copying
> > > uninitialized data from stack" "warning" { target { !
> > > default_packed
> > > } } } */
> > > +                       /* { dg-message "1 byte is uninitialized"
> > > "how much note" { target { ! default_packed } } .-1 } */
> > >                         err = -1;
> > >  
> > >         /* [...snip...] */
> > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-
> > > 2.c
> > > b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-2.c
> > > index 2096bda71798..d5f598b0878e 100644
> > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-2.c
> > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-2.c
> > > @@ -15,22 +15,22 @@ typedef unsigned short __u16;
> > >  
> > >  struct sco_conninfo {
> > >         __u16 hci_handle;
> > > -       __u8  dev_class[3]; /* { dg-message "padding after field
> > > 'dev_class' is uninitialized \\(1 byte\\)" } */
> > > +       __u8  dev_class[3]; /* { dg-message "padding after field
> > > 'dev_class' is uninitialized \\(1 byte\\)" "padding"  { target {
> > > !
> > > default_packed } } } */
> > >  };
> > >  
> > >  /* Adapted from sco_sock_getsockopt_old in net/bluetooth/sco.c. 
> > > */
> > >  
> > >  int test_1 (char __user *optval, const struct sco_conninfo *in)
> > >  {
> > > -       struct sco_conninfo cinfo; /* { dg-message "region
> > > created on
> > > stack here" "where" } */
> > > -                                  /* { dg-message "capacity: 6
> > > bytes" "capacity" { target *-*-* } .-1 } */
> > > +       struct sco_conninfo cinfo; /* { dg-message "region
> > > created on
> > > stack here" "where" { target { ! default_packed } } } */
> > > +                                  /* { dg-message "capacity: 6
> > > bytes" "capacity" { target { ! default_packed } } .-1 } */
> > >         /* Note: 40 bits of fields, padded to 48.  */
> > >  
> > >         cinfo.hci_handle = in->hci_handle;
> > >         memcpy(cinfo.dev_class, in->dev_class, 3);
> > >  
> > > -       copy_to_user(optval, &cinfo, sizeof(cinfo)); /* { dg-
> > > warning
> > > "potential exposure of sensitive information by copying
> > > uninitialized
> > > data from stack" "warning" } */
> > > -       /* { dg-message "1 byte is uninitialized" "how much note"
> > > {
> > > target *-*-* } .-1 } */
> > > +       copy_to_user(optval, &cinfo, sizeof(cinfo)); /* { dg-
> > > warning
> > > "potential exposure of sensitive information by copying
> > > uninitialized
> > > data from stack" "warning" { target { ! default_packed } } } */
> > > +       /* { dg-message "1 byte is uninitialized" "how much note"
> > > {
> > > target { ! default_packed } } .-1 } */
> > >  }
> > >  
> > >  int test_2 (char __user *optval, const struct sco_conninfo *in)
> > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18549-
> > > 1.c
> > > b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18549-1.c
> > > index 8a1c816cc1b5..8fcf9a904a2a 100644
> > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18549-1.c
> > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18549-1.c
> > > @@ -35,7 +35,7 @@ struct aac_srb_reply
> > >         __le32          scsi_status;
> > >         __le32          data_xfer_length;
> > >         __le32          sense_data_size;
> > > -       u8              sense_data[AAC_SENSE_BUFFERSIZE]; /* {
> > > dg-
> > > message "padding after field 'sense_data' is uninitialized \\(2
> > > bytes\\)" } */
> > > +       u8              sense_data[AAC_SENSE_BUFFERSIZE]; /* {
> > > dg-
> > > message "padding after field 'sense_data' is uninitialized \\(2
> > > bytes\\)" "padding" { target { ! default_packed } } } */
> > >  };
> > >  
> > >  #define                ST_OK           0
> > > @@ -50,8 +50,8 @@ static int aac_send_raw_srb(/* [...snip...] */
> > >  
> > >         /* [...snip...] */
> > >  
> > > -       struct aac_srb_reply reply; /* { dg-message "region
> > > created
> > > on stack here" "memspace message" } */
> > > -       /* { dg-message "capacity: 52 bytes" "capacity message" {
> > > target *-*-* } .-1 } */
> > > +       struct aac_srb_reply reply; /* { dg-message "region
> > > created
> > > on stack here" "memspace message" { target { ! default_packed } }
> > > }
> > > */
> > > +       /* { dg-message "capacity: 52 bytes" "capacity message" {
> > > target { ! default_packed } } .-1 } */
> > >  
> > >         reply.status = ST_OK;
> > >                 
> > > @@ -65,8 +65,8 @@ static int aac_send_raw_srb(/* [...snip...] */
> > >  
> > >         /* [...snip...] */
> > >  
> > > -       if (copy_to_user(user_reply, &reply, /* { dg-warning
> > > "potential exposure of sensitive information by copying
> > > uninitialized
> > > data from stack" } */
> > > -                                            /* { dg-message "2
> > > bytes
> > > are uninitialized" "note how much" { target *-*-* } .-1 } */
> > > +       if (copy_to_user(user_reply, &reply, /* { dg-warning
> > > "potential exposure of sensitive information by copying
> > > uninitialized
> > > data from stack" "padding" { target { ! default_packed } } } */
> > > +                                            /* { dg-message "2
> > > bytes
> > > are uninitialized" "note how much" { target { ! default_packed }
> > > } .-
> > > 1 } */
> > >                          sizeof(struct aac_srb_reply))) {
> > >                 /* [...snip...] */
> > >         }
> > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18550-
> > > 1.c
> > > b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18550-1.c
> > > index 4272da96bab0..a7361f937401 100644
> > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18550-1.c
> > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18550-1.c
> > > @@ -31,7 +31,7 @@ struct aac_hba_info {
> > >         u8      driver_name[50]; /* { dg-message "field
> > > 'driver_name'
> > > is uninitialized \\(50 bytes\\)" } */
> > >         u8      adapter_number;
> > >         u8      system_io_bus_number;
> > > -       u8      device_number; /* { dg-message "padding after
> > > field
> > > 'device_number' is uninitialized \\(3 bytes\\)" } */
> > > +       u8      device_number; /* { dg-message "padding after
> > > field
> > > 'device_number' is uninitialized \\(3 bytes\\)" "padding" {
> > > target {
> > > ! default_packed } } } */
> > >         u32     function_number;
> > >         u32     vendor_id;
> > >         u32     device_id;
> > > @@ -108,7 +108,8 @@ struct pci_bus {
> > >  static int aac_get_hba_info(struct aac_dev *dev, void __user
> > > *arg)
> > >  {
> > >         struct aac_hba_info hbainfo; /* { dg-message "region
> > > created
> > > on stack here" "memspace message" } */
> > > -       /* { dg-message "capacity: 200 bytes" "capacity message"
> > > {
> > > target *-*-* } .-1 } */
> > > +       /* { dg-message "capacity: 200 bytes" "capacity message"
> > > {
> > > target { ! default_packed } } .-1 } */
> > > +       /* { dg-message "capacity: 194 bytes" "capacity message"
> > > {
> > > target default_packed } .-2 } */
> > >  
> > >         hbainfo.adapter_number          = (u8) dev->id;
> > >         hbainfo.system_io_bus_number    = dev->pdev->bus->number;
> > > @@ -121,7 +122,7 @@ static int aac_get_hba_info(struct aac_dev
> > > *dev,
> > > void __user *arg)
> > >         hbainfo.sub_system_id           = dev->pdev-
> > > > subsystem_device;
> > >  
> > >         if (copy_to_user(arg, &hbainfo, sizeof(struct
> > > aac_hba_info)))
> > > { /* { dg-warning "potential exposure of sensitive information by
> > > copying uninitialized data from stack" "warning" } */
> > > -               /* { dg-message "177 bytes are uninitialized"
> > > "how
> > > much" { target *-*-* } .-1 } */
> > > +               /* { dg-message "177 bytes are uninitialized"
> > > "how
> > > much" { target { ! default_packed } } .-1 } */
> > >                 /* [...snip...] */
> > >         }
> > >  
> > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-antipatterns-
> > > 1.c
> > > b/gcc/testsuite/gcc.dg/plugin/infoleak-antipatterns-1.c
> > > index 500845364388..d9db2b3bdebc 100644
> > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-antipatterns-1.c
> > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-antipatterns-1.c
> > > @@ -90,21 +90,21 @@ int infoleak_heap_missing_a_field(void __user
> > > *dst, u32 v)
> > >  
> > >  struct infoleak_3
> > >  {
> > > -  u8 a; /* { dg-message "padding after field 'a' is
> > > uninitialized
> > > \\(3 bytes\\)" } */
> > > +  u8 a; /* { dg-message "padding after field 'a' is
> > > uninitialized
> > > \\(3 bytes\\)" "padding" { target { ! default_packed } } } */
> > >    /* padding here */
> > >    u32 b;
> > >  };
> > >  
> > >  int infoleak_stack_padding(void __user *dst, u8 p, u32 q)
> > >  {
> > > -  struct infoleak_3 st; /* { dg-message "region created on stack
> > > here" "where" } */
> > > -  /* { dg-message "capacity: 8 bytes" "capacity" { target *-*-*
> > > } .-
> > > 1 } */
> > > +  struct infoleak_3 st; /* { dg-message "region created on stack
> > > here" "where" { target { ! default_packed } } } */
> > > +  /* { dg-message "capacity: 8 bytes" "capacity" { target { !
> > > default_packed } } .-1 } */
> > >  
> > >    st.a = p;
> > >    st.b = q;
> > >    /* No initialization of padding.  */
> > > -  if (copy_to_user(dst, &st, sizeof(st))) /* { dg-warning
> > > "potential
> > > exposure of sensitive information by copying uninitialized data
> > > from
> > > stack" "warning" } */
> > > -    /* { dg-message "3 bytes are uninitialized" "note how much"
> > > {
> > > target *-*-* } .-1 } */
> > > +  if (copy_to_user(dst, &st, sizeof(st))) /* { dg-warning
> > > "potential
> > > exposure of sensitive information by copying uninitialized data
> > > from
> > > stack" "warning" { target { ! default_packed } } } */
> > > +    /* { dg-message "3 bytes are uninitialized" "note how much"
> > > {
> > > target { ! default_packed } } .-1 } */
> > >      return -EFAULT;
> > >    return 0;
> > >  }
> > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c
> > > b/gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c
> > > index 6961b44f76b9..192c9e7802ad 100644
> > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c
> > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c
> > > @@ -11,16 +11,18 @@ typedef unsigned int u32;
> > >  
> > >  struct st
> > >  {
> > > -  u8 i;  /* { dg-message "padding after field 'i' is
> > > uninitialized
> > > \\(3 bytes\\)" } */
> > > +  u8 i;  /* { dg-message "padding after field 'i' is
> > > uninitialized
> > > \\(3 bytes\\)" "padding" { target { ! default_packed } } } */
> > >    u32 j; /* { dg-message "field 'j' is uninitialized \\(4
> > > bytes\\)"
> > > } */
> > >  };
> > >  
> > >  void test (void __user *dst, u8 a)
> > >  {
> > >    struct st s; /* { dg-message "region created on stack here"
> > > "where" } */
> > > -  /* { dg-message "capacity: 8 bytes" "capacity" { target *-*-*
> > > } .-
> > > 1 } */
> > > -  /* { dg-message "suggest forcing zero-initialization by
> > > providing
> > > a '.0.' initializer" "fix-it hint" { target *-*-* } .-2 } */  
> > > +  /* { dg-message "capacity: 8 bytes" "capacity" { target { !
> > > default_packed } } .-1 } */
> > > +  /* { dg-message "capacity: 5 bytes" "capacity" { target
> > > default_packed } .-2 } */
> > > +  /* { dg-message "suggest forcing zero-initialization by
> > > providing
> > > a '.0.' initializer" "fix-it hint" { target *-*-* } .-3 } */
> > >    s.i = a;
> > >    copy_to_user(dst, &s, sizeof (struct st)); /* { dg-warning
> > > "potential exposure of sensitive information by copying
> > > uninitialized
> > > data from stack" "warning" } */
> > > -  /* { dg-message "7 bytes are uninitialized" "note how much" {
> > > target *-*-* } .-1 } */
> > > +  /* { dg-message "7 bytes are uninitialized" "note how much" {
> > > target { ! default_packed } } .-1 } */
> > > +  /* { dg-message "4 bytes are uninitialized" "note how much" {
> > > target default_packed } .-2 } */
> > >  }
> > 
>