Date: Thu, 9 Oct 2003 22:12: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: <20031009213822.L9492@gamplex.bde.org> In-Reply-To: <20031009200740.R9199@gamplex.bde.org> References: <200310090748.h997m4YJ018758@grimreaper.grondar.org> <20031009200740.R9199@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 9 Oct 2003, Bruce Evans wrote:
> > Hiroki Sato writes:
> > > Could anyone please review a patch in bin/56502? That fixes a bug
> > > in random.c that causes memory corruption.
> 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).]
> ...
> /*
> * 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'.]
> %%%
>
> The "28 December 1994" changes were made in rev.1.5 (Lite2 merge).
Bah, the alignment problem is secondary. I naively assumed that the
bug wasn't related to longs being larger than ints since it hasn't
been reported for other machines, but in fact it causes buffer overrun
by a factor of sizeof(long)/4 on all machines where this factor is > 1
(I tested on alphas). This is fixed in NetBSD. Our rev.1.5 is
mostly wrong. NetBSD obtained the same bug by importing Lite2 in their
rev.1.7, but fixed it in their rev.1.20. Some diffs between NetBSD 1.7
and 1.22:
112a119,123
> *
> * Modified 07 January 2002 by Jason R. Thorpe.
> * The following changes have been made:
> * All the references to "long" have been changed back to "int". This
> * fixes memory corruption problems on LP64 platforms.
This basically moves the bug from the large set of machines where
sizeof(long) != 4 to the small set of machines where sizeof(int) != 4.
s/int/int32_t/ not quite as in the PR would be a more complete fix for
the main problem, but I don't like it since it puts magic 32's all over
the code. The bug is caused by magic 4's not even literally present:
% if (n < BREAK_1) {
% rand_type = TYPE_0;
% rand_deg = DEG_0;
% rand_sep = SEP_0;
% } else if (n < BREAK_2) {
% rand_type = TYPE_1;
% rand_deg = DEG_1;
% rand_sep = SEP_1;
% } else if (n < BREAK_3) {
% rand_type = TYPE_2;
% rand_deg = DEG_2;
% rand_sep = SEP_2;
% } else if (n < BREAK_4) {
% rand_type = TYPE_3;
% rand_deg = DEG_3;
% rand_sep = SEP_3;
% } else {
% rand_type = TYPE_4;
% rand_deg = DEG_4;
% rand_sep = SEP_4;
% }
% state = (long *) (long_arg_state + 1); /* first location */
% end_ptr = &state[rand_deg]; /* must set end_ptr before srandom */
Here BREAK_N are literal constsants, but they should be related to
sizeof(state[0]). E.g., BREAK_4 is 256, which is enough for 64 32-bit
longs, but it needs to be enough for 64 elements in the state array,
so it isn't enough for 64-bit longs.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20031009213822.L9492>