Date: Thu, 9 Oct 2003 21:08:30 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Mark Murray <mark@grondar.org> Cc: Hiroki Sato <hrs@eos.ocn.ne.jp> Subject: Re: bin/56502 Message-ID: <20031009200740.R9199@gamplex.bde.org> In-Reply-To: <200310090748.h997m4YJ018758@grimreaper.grondar.org> References: <200310090748.h997m4YJ018758@grimreaper.grondar.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20031009200740.R9199>