[Bug-ed] Aliasing violations in ed cause segfaults.

[Harald van Dijk]

Hi,

Aliasing violations are a fun thing. They are found by the
compiler, the compiler warns the user, the user does not see a
direct problem, and modifies the code so that the compiler doesn't
warn about it anymore. Now comes a newer compiler, with newer
optimisations, and the incorrect code does cause problems, but the
warning about it is still suppressed. *Please* don't do that unless
the code is actually correct.

In this case, the first problem is in carg_parser.[ch]:

typedef struct
  {
  ap_Record * data;
  char * error;
  int data_size;
  int error_size;
  }
Arg_parser;

char ap_resize_buffer( char **buf, int *size, const int min_size );

char push_back_record( Arg_parser * ap, const int code, const char * argument )
  {
/* ... */
  if( !ap_resize_buffer( (char **)(void *)&(ap->data), 0,
                         ( ap->data_size + 1 ) * sizeof( ap_Record ) ) )
    return 0;
/* ... */
  }

ap->data is a pointer to an ap_Record. If the cast to void * is
removed, GCC can warn about it:

carg_parser.c: In function ‘push_back_record’:
carg_parser.c:44: warning: dereferencing type-punned pointer will break 
strict-aliasing rules

With the cast, GCC is told to shut up about it, but still optimises
on the assumption that ap->data is never actually accessed as a
char *.

Now, for most systems, that doesn't matter. The optimiser is not
smart enough to actually do anything with that knowledge. But a
user found a combination of compiler and compiler flags that change
that. When ed is compiled with GCC 4.2 (4.2.2 or 4.2.3, at least), and
the compiler flags include
  -march=pentium-m
  -O2
  -fomit-frame-pointer
  -finline-functions
  -fpeel-loops
or these options are enabled for other reasons (such as -O3 or
-fprofile-use), ed fails at parsing command-line options. Horribly.
In fact, if an attempt is made to run the testsuite, literally
every single test fails.

It's not just carg_parser, either. Although carg_parser is the
reason ed bombs here, as pointed out by gdb:

Program received signal SIGSEGV, Segmentation fault.
push_back_record (ap=0xffd644a8, code=0, argument=0xffd65176 "/dev/null") at 
carg_parser.c:48
48        p->code = code;
(gdb) bt
#0  push_back_record (ap=0xffd644a8, code=0, argument=0xffd65176 "/dev/null") 
at carg_parser.c:48
#1  0x0804cb83 in ap_init (ap=0xffd644a8, argc=2, argv=0xffd64564, 
options=0x80531e0, in_order=0 '\0') at carg_parser.c:244
#2  0x0804dee7 in main (argc=Cannot access memory at address 0x0
) at main.c:150
(gdb) p p
$1 = (ap_Record *) 0x0

...similar aliasing violations exist in other parts of the program as
well.

A fix requires either defining every pointer that's accessed as a
char * as such, or not accessing those pointers as a char *. The
former is easy but horribly ugly. It's so ugly that I initially
decided to not mail you about the problem just yet. If you want to
see it anyway, I've attached it to the Gentoo bugreport, and it's
available at
  http://bugs.gentoo.org/attachment.cgi?id=142426&action=view
The latter, not accessing those pointers as a char * would probably
be a better solution. However, taking that approach requires
reworking the resize_buffer logic. For ap_resize_buffer, that's not
so much of a problem (return a void *, let the caller convert it to
the proper type, like realloc), but for resize_buffer, it is, because
of the disable_ and enable_interrupts calls. I haven't been able to
fix it myself in that approach in a clean way, but I hope you have
better luck.

(Of course, there is a third possibility: unconditionally force
-fno-strict-aliasing in the compiler options. This would work, but
it may break compilation with other compilers if they make the same
optimisation as GCC, but have different command-line options, or no
command-line options, to disable it.)

If there's anything I can do to help, please let me know. And
thanks for looking after ed. I'm glad someone does, because I like
it. :)