bin/56502

[Bruce Evans]

On Thu, 9 Oct 2003, Mark Murray wrote:

> Hiroki Sato writes:
> >  Could anyone please review a patch in bin/56502?  That fixes a bug
> >  in random.c that causes memory corruption.
>
> It looks fine to me. Has it been tested on all platforms?

It changes far too much and increases type mismatch problems.

The test program gives undefined behaviour.  From random.c:

%%%
 * Modified 28 December 1994 by Jacob S. Rosenberg.
 * The following changes have been made:
 * All references to the type u_int have been changed to unsigned long.
 * All references to type int have been changed to type long.  Other

[The proposed fix sort of reverts this, except it mostly uses uint32_t,
which may be better but may cause sign extension problems.  Places that
use plain int are broken on machines where sizeof(int) < sizeof(int32_t).]

 * cleanups have been made as well.  A warning for both initstate and
 * setstate has been inserted to the effect that on Sparc platforms
 * the 'arg_state' variable must be forced to begin on word boundaries.

[I can't see the warning, except in this comment.]

 * This can be easily done by casting a long integer array to char *.

[This is an (undocumented except here :-() requirement on the caller of
initstate().]

...
/*
 * initstate:
...
 * Note: The Sparc platform requires that arg_state begin on a long
 * word boundary; otherwise a bus error will occur. Even so, lint will
 * complain about mis-alignment, but you should disregard these messages.
 */

[The test program doesn't do this.  It has 'static char b[256]' followed
by 'static int *c' and passes &b[0].  Alignment of the pointer apparently
increases it, so using all of the 256 bytes following the aligned pointer
overruns `b'.  The increase is apparently enough for the overrun to reach
'c'.]

char *
initstate(seed, arg_state, n)
	unsigned long seed;		/* seed for R.N.G. */
	char *arg_state;		/* pointer to state array */
	long n;				/* # bytes of state info */
{
	char *ostate = (char *)(&state[-1]);
	long *long_arg_state = (long *) arg_state;

[This bogus casts hides the bug.]

	if (rand_type == TYPE_0)
		state[-1] = rand_type;
	else
		state[-1] = MAX_TYPES * (rptr - state) + rand_type;
	if (n < BREAK_0) {
		(void)fprintf(stderr,
		    "random: not enough state (%ld bytes); ignored.\n", n);
		return(0);
	}
...
[Similarly for setstate().]
%%%

The "28 December 1994" changes were made in rev.1.5 (Lite2 merge).

The patch in the PR uses a different bogus cast:

	uint32_t *int_arg_state = (uint32_t *)(void *)arg_state;

This apparently works by reducing the alignment requirements a little.
Howver, I'm surprised that aligning `b' actually clobbers `c' in the
test program:

	static int *a;
	static char b[256];
	static int *c;

I would have thought that this gave 'a', b and c following each other in
memory, with all of them very aligned because things are naturally
aligned following 'a' and 256 is a large enough power of 2.  The following
would force misalignement on most machines:

	struct {
		long	align;
		char	misalign[sizeof(long) - 1];
		char	misaligned_state[256];
		char	always_clobbered;
	};

For a quick fix, I would print a message and return 0 if
(void *)long_arg_state != (void *)arg_state.

Bruce