Re: [PATCH v2] tftp.h: rework layout to work with fortification

[Mike Frysinger <vapier at gentoo dot org>]

On Sunday 06 May 2012 00:12:39 Carlos O'Donell wrote:
> On Thu, Apr 12, 2012 at 1:30 PM, Mike Frysinger wrote:
> > --- a/inet/arpa/tftp.h
> > +++ b/inet/arpa/tftp.h
> > @@ -49,16 +49,17 @@
> >  struct tftphdr {
> >        short   th_opcode;
> >        union {
> > -               unsigned short  tu_block;
> > -               short   tu_code;
> > -               char    tu_stuff[1];
> > -       } __attribute__ ((__packed__)) th_u;
> > -       char    th_data[1];
> > +               struct {
> > +                       union {
> > +                               unsigned short  th_block;
> > +                               short   th_code;
> > +                       } __attribute__ ((__packed__));
> > +                       char th_data[0];
> > +               } __attribute__ ((__packed__));
> > +               char    th_stuff[0];
> > +       } __attribute__ ((__packed__));
> >  } __attribute__ ((__packed__));
> 
> I understand that fortification causes some problems here, but...
> 
> Doesn't that break the ABI?

how could it break ABI ?  there are no funcs in glibc that take this struct as 
an argument or return it.  i don't think there are any files that even include 
this header.

as for random user code that includes this header, you don't malloc the struct 
or declare it on the stack because of the nature of it -- you overlay this on 
top of an arbitrarily long buffer that you received are are going to be 
sending.  sizeof() is now 4 when it was 5, but that only affects things that 
want to access th_data, and that wouldn't make sense if you tried to 
instantiate the struct directly (e.g. struct tftphdr hdr;) since th_data, 
accordingly to the spec, has no defined length.

as for the offset's of the members, those are all the same.

> > -#define        th_block        th_u.tu_block
> > -#define        th_code         th_u.tu_code
> > -#define        th_stuff        th_u.tu_stuff
> >  #define        th_msg          th_data
> 
> Why do you remove the defines for th_block, th_code and th_stuff?
> Isn't it possible that defines are used by userspace code?

look at the struct -- i moved the names there
-mike

Attachment: signature.asc
Description: This is a digitally signed message part.