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

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Mon, May 7, 2012 at 1:43 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> The current tftp structure does not work when fortification is enabled.
> Starting with gcc-4.5, more size checking was added to trigger these.
> Older versions just didn't have enough information, so they returned -1
> as the sizes.
>
> First, the tu_stuff field is declared as 1 byte (when it's really an
> arbitrary length C string), so attempting to strcpy() with it results
> in crashes. ?This fails with _FORTIFY_SOURCE=1.
>
> Second, even if we change that to [0] (since gcc does not allow flexible
> array members in an union), gcc is not smart enough to see that they are
> two overlapping flexible arrays (tu_stuff and tu_data), so it will still
> trigger an abort with _FORTIFY_SOURCE=2. ?This is because it thinks that
> tu_stuff is 0 bytes and tu_data comes after it.
>
> Talking to upstream gcc, they don't seem terribly inclined to fix the
> 2nd issue, but even if they did, we still have plenty of 4.5 and 4.6
> installs that would hit problems.
>
> So, let's re-order with a few more anonymous structs & unions so that
> the fields are laid out with a zero-length array always as the last
> field. ?This seems to fix things with gcc-4.6, and the tftp-hpa pkg
> continues to build & work.
>
> URL: https://bugs.launchpad.net/ubuntu/+source/tftp-hpa/+bug/691345
> URL: https://bugs.archlinux.org/task/28103
> URL: https://bugs.gentoo.org/357083
> URL: http://gcc.gnu.org/PR52944
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
> 2012-04-12 ?Mike Frysinger ?<vapier@gentoo.org>
>
> ? ? ? ?* inet/arpa/tftp.h (struct tftphdr): Rename th_u to th_u1. ?Add
> ? ? ? ?a struct th_u2 inside the union, and move tu_block/tu_code into
> ? ? ? ?a new th_u3 union of tu_block/tu_code inside of that. ?Move
> ? ? ? ?th_data[1] into the th_u2 as tu_data[0]. ?Change tu_stuff[1] to
> ? ? ? ?tu_stuff[0]. ?Add a new tu_padding[4] to keep sizeof() the same.
> ? ? ? ?(th_block): Change to th_u1.th_u2.th_u3.tu_block.
> ? ? ? ?(th_code): Change to th_u1.th_u2.th_u3.tu_code.
> ? ? ? ?(th_stuff): Change to th_u1.tu_stuff.
> ? ? ? ?(th_data): Define.
> ? ? ? ?(th_msg): Change to th_u1.th_u2.tu_data.
> ---
> v3
> ? ? ? ?- use named unions/structs
> ? ? ? ?- add a hack to keep sizeof() the same

This is much better.

The size is the same.

The offsets are the same.

It works with fortification.

> ?inet/arpa/tftp.h | ? 24 +++++++++++++++---------
> ?1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/inet/arpa/tftp.h b/inet/arpa/tftp.h
> index 21b0559..86e0b6e 100644
> --- a/inet/arpa/tftp.h
> +++ b/inet/arpa/tftp.h
> @@ -49,17 +49,23 @@
> ?struct tftphdr {
> ? ? ? ?short ? th_opcode; ? ? ? ? ? ? ? ? ? ? ?/* packet type */
> ? ? ? ?union {
> - ? ? ? ? ? ? ? unsigned short ?tu_block; ? ? ? /* block # */
> - ? ? ? ? ? ? ? short ? tu_code; ? ? ? ? ? ? ? ?/* error code */
> - ? ? ? ? ? ? ? char ? ?tu_stuff[1]; ? ? ? ? ? ?/* request packet stuff */
> - ? ? ? } __attribute__ ((__packed__)) th_u;
> - ? ? ? char ? ?th_data[1]; ? ? ? ? ? ? ? ? ? ? /* data or error string */
> + ? ? ? ? ? ? ? char ? ?tu_padding[3]; ? ? ? ? ?/* sizeof() compat */
> + ? ? ? ? ? ? ? struct {
> + ? ? ? ? ? ? ? ? ? ? ? union {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned short ?tu_block; ? ? ? /* block # */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? short ? tu_code; ? ? ? ? ? ? ? ?/* error code */
> + ? ? ? ? ? ? ? ? ? ? ? } __attribute__ ((__packed__)) th_u3;
> + ? ? ? ? ? ? ? ? ? ? ? char tu_data[0]; ? ? ? ?/* data or error string */
> + ? ? ? ? ? ? ? } __attribute__ ((__packed__)) th_u2;
> + ? ? ? ? ? ? ? char ? ?tu_stuff[0]; ? ? ? ? ? ?/* request packet stuff */
> + ? ? ? } __attribute__ ((__packed__)) th_u1;
> ?} __attribute__ ((__packed__));

It still doesn't work with -Wall -Werror -pedantic because zero sized
arrays are not allowed in ISO C.

I know that this getting uglier by the minute, but I really don't like
the fact that we used to compile without error and now we don't.

I don't care what the header looks like as long as source
compatibility and ABI are maintained.

I would prefer something like this:
~~~
...
/* There is no way for fortification and ISO C90/99 to define a structure
   such that it is valid and working for both. ISO C doesn't support zero
   length arrays, and fortification doesn't support lying about arrays
   that are actually variable length but declared as 1 char. The optimal
   solution would be a C99 variable length array but the time of writing
   the support is insufficiently flexible to be used in this structure.
   Thus we change the definition of the structure slightly under fortification
   which will break applications compiling with -Werror -Wall -pedantic,
   but this is the best we can do while maintaining backwards compatibility
   with applications that don't use fortification.  */
# if (defined _FORTIFY_SOURCE) && (_FORTIFY_SOURCE > 0)
                       char tu_data[0];        /* data or error string */
# else
                       char tu_data[1];
# endif
               } __attribute__ ((__packed__)) th_u2;
# if (defined _FORITYF_SOURCE) && (_FORTIFY_SOURCE > 0)
               char    tu_stuff[0];            /* request packet stuff */
# else
               char    tu_stuff[1];
# endif
...
~~~

That way we have:

* Without fortification we maintain the previous behaviour.
* With fortification you suffer from a warning about zero length
arrays. Nothing we can do about it.

Comments?

Cheers,
Carlos.