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

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

On Thu, Apr 12, 2012 at 1:30 PM, 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): Add a struct inside the union,
> ? ? ? ?and move tu_block/tu_code into a union of th_block/th_code inside
> ? ? ? ?of that. ?Move th_data[1] to the inner struct as th_data[0].
> ? ? ? ?Change tu_stuff[1] to th_stuff[0]. ?Delete th_u union name.
> ? ? ? ?(th_block): Delete
> ? ? ? ?(th_code, th_stuff): Likewise.
> ---
> v2
> ? ? ? ?- maintain packed in all unions/structs
>
> ?inet/arpa/tftp.h | ? 17 +++++++++--------
> ?1 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/inet/arpa/tftp.h b/inet/arpa/tftp.h
> index 21b0559..f1baa30 100644
> --- a/inet/arpa/tftp.h
> +++ b/inet/arpa/tftp.h
> @@ -49,16 +49,17 @@
> ?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 */
> + ? ? ? ? ? ? ? struct {
> + ? ? ? ? ? ? ? ? ? ? ? union {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned short ?th_block; ? ? ? /* block # */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? short ? th_code; ? ? ? ? ? ? ? ?/* error code */
> + ? ? ? ? ? ? ? ? ? ? ? } __attribute__ ((__packed__));
> + ? ? ? ? ? ? ? ? ? ? ? char th_data[0]; ? ? ? ?/* data or error string */
> + ? ? ? ? ? ? ? } __attribute__ ((__packed__));
> + ? ? ? ? ? ? ? char ? ?th_stuff[0]; ? ? ? ? ? ?/* request packet stuff */
> + ? ? ? } __attribute__ ((__packed__));
> ?} __attribute__ ((__packed__));

I understand that fortification causes some problems here, but...

Doesn't that break the ABI?

Can you show me that it doesn't?

> -#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?

Cheers,
Carlos.