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

[Mike Frysinger]

On Sunday 06 May 2012 18:28:37 Carlos O'Donell wrote:
> The ABI is comprised of two things:
> 
> * The layout.
> 
> * The size.

the size matters to the ABI, if the ABI utilizes those structs.  glibc doesn't 
use it anywhere, so i don't think it's accurate to describe this struct as 
being part of glibc's ABI.

> Example 1:
> ~~~
> ...
> int tftp_send_ack(struct tftphdr *tftp_hdr, int block)

i don't think this code is userspace code.  especially considering they use 
sk_buffs.  going by google, that's BIOS code.

> Example 2:
> ~~~
> ...
> char data_out[SEGSIZE+sizeof(struct tftphdr)];
> char data_in[SEGSIZE+sizeof(struct tftphdr)];
> ...
> static void
> tftpd_write_file(struct tftphdr *hdr,
>                  struct sockaddr_in *from_addr, int from_len)
> {

and this is from FreeRTOS/LWIP.

examples that actually use the header would be tftp-hpa or netkit-tftp or 
atftp, and from grepping those source trees, their usage is as i described -- 
no one uses sizeof().  there is only one place where the struct is declared on 
the stack and that too is as i described -- it only uses it for the header 
structure and manually writes out 4 bytes (per the spec) to the network.

> * Application allocates N-1 bytes because it's using the new struct
> tfpthdr. * Library copies N bytes because it's using the old struct
> tftphdr. * Library crashes because the Nth byte is not a valid location.

yes, but i don't think that's a realistic use case as it doesn't make sense.  
the tftphdr struct is used to access the first 4 bytes only.  everything after 
that is arbitrary.

> You need to add padding to make the sizeof come out the same.

i don't think we do. 

> >> > -#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
> 
> You used anonymous structures and unions which are not valid ISO C90
> or C99 (though I *think* they are valid ISO C11).

yes ... that's a trivial matter to address if it's something we care about.  
GCC says they're valid in ISO C11:
	http://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html
and certainly gcc itself has supported this for over a decade.  not that we 
want to make our headers parsable only by gcc.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20120507/9c9d62b9/attachment.sig>