svn commit: r278737 - head/usr.sbin/flowctl

[Bruce Evans]

On Fri, 13 Feb 2015, Gleb Smirnoff wrote:

> Author: glebius
> Date: Fri Feb 13 23:57:20 2015
> New Revision: 278737
> URL: https://svnweb.freebsd.org/changeset/base/278737
>
> Log:
>  Use less ugly code to allocate buffer of SORCVBUF_SIZE.

Less ugly, but wrong.  The version that used alloca() was correct.

> Modified: head/usr.sbin/flowctl/flowctl.c
> ==============================================================================
> --- head/usr.sbin/flowctl/flowctl.c	Fri Feb 13 23:43:59 2015	(r278736)
> +++ head/usr.sbin/flowctl/flowctl.c	Fri Feb 13 23:57:20 2015	(r278737)
> @@ -222,10 +222,12 @@ ctl_show(int argc, char **argv)
> static void
> do_show(int version, void (*func)(struct ngnf_show_header *))
> {
> -	struct ng_mesg ng_mesg[SORCVBUF_SIZE];
> +	char buf[SORCVBUF_SIZE];

alloca(), like malloc(), gave a buffer suitably aligned for any object.
This only gives a buffer suitably aligned for char objects.  It may
accidentally be suitably aligned for other objects.  The accident often
happens because objects on the stack are usually given larger alignment
than necessary.  Depending on this is unportable at best.

> +	struct ng_mesg *ng_mesg;
> 	struct ngnf_show_header req, *resp;
> 	int token, nread;
>
> +	ng_mesg = (struct ng_mesg *)buf;

The new bug is detected at high warning levels.  WARNS >= 4 gives
-Wcast-align unless the MK option to break this warning is configured.
The bug is detected by -Wcast0align even on amd64.

The bug is not detected by default because flowctl has many other
warnings at the default WARNS of 6 (mainly -Wcast-align and
-Wsign-compare ones), so it breaks the warnings using WARNGS?=2.

I think arches with strict alignment requirements have a warning about
this without -Wcast-align, but couldn't find one on ia64.  Certainly,
related warnings turned up on ia64 when they didn't on amd64 with the
same WARNS.

The runtime bug can be fixed using __aligned(__ALIGN_MUMBLE).  This
exposes a bug in -Wcast-align -- it still warns although the char buffer
is obviously aligned.  Compilers should also know that the buffer is
suitably aligned when they just allocated it on the stack and the
alignment happens to be enough.  But in this case, compilers should
also know that the suitable alignment is only accidental, and still
warn unless portability warnings are supressed.

This and the non-spurious warning can be broken using another cast:

 	ng_mesg = (struct ng_mesg *)(void *)buf;

This depends on the compiler being too stupid to remember the alignment
of the original char buffer.

This fixed version is still worse than the old one using alloca(),
because it is longer and more complicated.  alloca(), like malloc(),
returns a void * so that the compiler cannot know the alignment of the buffer
   (except it can for alloca() because it just allocated the buffer -- it
    must do a (void *) cast internally and forget the actual alignment,
    just like the above cast does but with more intentional forgetfulness).
We are basically using a home made alloca(N) for the easier case where N
is constant.

Using VLAs and also the C99 feature of declarations anwhere, and extensions
like __aligned(), we can almost implement a full alloca() using the fixed
version of this change:

/*
  * XXX need extended statement-expression so that __buf doesn't go out
  * of scope after the right brace.
  */
#define	my_alloca(n) __extension__ ({
 	/* XXX need unique name. */				\
 	char __buf[__roundup2((n), MUMBLE)] __aligned(MUMBLE);	\
 								\
 	(void *)__buf;						\
})

Bruce