Created attachment 14947 [details] test case foo1.c On Linux/SPARC (both in 64-bit mode and in 32-bit mode), the initstate() and initstate_r() functions crash when the state argument is unaligned. The specification of initstate() in https://pubs.opengroup.org/onlinepubs/9699919799/functions/initstate.html and the documentation of initstate() and initstate_r() in https://www.gnu.org/software/libc/manual/html_node/BSD-Random.html require merely a byte array. So, passing an aligned byte array (of size 128 or more) is allowed. How to reproduce: Remote login to cfarm105.cfarm.net. $ gcc -Wall foo1.c $ ./a.out Bus error $ gcc -m32 -Wall foo1.c $ ./a.out Bus error $ gcc -Wall foo2.c $ ./a.out Bus error $ gcc -m32 -Wall foo2.c $ ./a.out Bus error
Created attachment 14948 [details] test case foo2.c
I meant: Passing an *unaligned* byte array is allowed.
This is inherited from BSD, which documents that the state array needs to be uint32_t aligned.
I've just run into this as we're trying to test libunistring-1.2 downstream. What's the consensus here?
azanella, would you be kind enough to look? I would be happy to try but https://sourceware.org/bugzilla/show_bug.cgi?id=30584#c3 makes me fear that it's not clear how to fix it at all without breaking ABI.
(In reply to Sam James from comment #5) > azanella, would you be kind enough to look? I would be happy to try but > https://sourceware.org/bugzilla/show_bug.cgi?id=30584#c3 makes me fear that > it's not clear how to fix it at all without breaking ABI. I think the way to fix unaligned access without breaking the API is internally use memcpy or packed struct: diff --git a/stdlib/random_r.c b/stdlib/random_r.c index b6297fe099..dee3bc9535 100644 --- a/stdlib/random_r.c +++ b/stdlib/random_r.c @@ -55,6 +55,7 @@ #include <limits.h> #include <stddef.h> #include <stdlib.h> +#include <string.h> /* An improved random number generation package. In addition to the standard @@ -146,7 +147,19 @@ static const struct random_poly_info random_poly_info = { DEG_0, DEG_1, DEG_2, DEG_3, DEG_4 } }; +static inline int32_t +read_state (int32_t *b, size_t idx) +{ + int32_t r; + memcpy (&r, &b[idx], sizeof (int32_t)); + return r; +} +static inline void +write_state (int32_t *b, size_t idx, int32_t v) +{ + memcpy (&b[idx], &v, sizeof (int32_t)); +} /* Initialize the random number generator based on the given seed. If the @@ -177,7 +190,7 @@ __srandom_r (unsigned int seed, struct random_data *buf) /* We must make sure the seed is not 0. Take arbitrarily 1 in this case. */ if (seed == 0) seed = 1; - state[0] = seed; + write_state (state, 0, seed); if (type == TYPE_0) goto done; @@ -194,7 +207,7 @@ __srandom_r (unsigned int seed, struct random_data *buf) word = 16807 * lo - 2836 * hi; if (word < 0) word += 2147483647; - *++dst = word; + write_state (++dst, 0, word); } buf->fptr = &state[buf->rand_sep]; @@ -238,9 +251,10 @@ __initstate_r (unsigned int seed, char *arg_state, size_t n, { int old_type = buf->rand_type; if (old_type == TYPE_0) - old_state[-1] = TYPE_0; + write_state (old_state, -1, TYPE_0); else - old_state[-1] = (MAX_TYPES * (buf->rptr - old_state)) + old_type; + write_state (old_state, -1, (MAX_TYPES * (buf->rptr - old_state)) + + old_type); } int type; @@ -270,9 +284,9 @@ __initstate_r (unsigned int seed, char *arg_state, size_t n, __srandom_r (seed, buf); - state[-1] = TYPE_0; + write_state (state, -1, TYPE_0); if (type != TYPE_0) - state[-1] = (buf->rptr - state) * MAX_TYPES + type; + write_state (state, -1, (buf->rptr - state) * MAX_TYPES + type); return 0; @@ -307,9 +321,10 @@ __setstate_r (char *arg_state, struct random_data *buf) old_type = buf->rand_type; old_state = buf->state; if (old_type == TYPE_0) - old_state[-1] = TYPE_0; + write_state (old_state, -1, TYPE_0); else - old_state[-1] = (MAX_TYPES * (buf->rptr - old_state)) + old_type; + write_state (old_state, -1, (MAX_TYPES * (buf->rptr - old_state)) + + old_type); type = new_state[-1] % MAX_TYPES; if (type < TYPE_0 || type > TYPE_4) @@ -361,8 +376,8 @@ __random_r (struct random_data *buf, int32_t *result) if (buf->rand_type == TYPE_0) { - int32_t val = ((state[0] * 1103515245U) + 12345U) & 0x7fffffff; - state[0] = val; + int32_t val = ((read_state(state, 0) * 1103515245U) + 12345U) & 0x7fffffff; + write_state (state, 0, val); *result = val; } else @@ -372,7 +387,9 @@ __random_r (struct random_data *buf, int32_t *result) int32_t *end_ptr = buf->end_ptr; uint32_t val; - val = *fptr += (uint32_t) *rptr; + val = read_state (rptr, 0); + int32_t t = read_state (fptr, 0); + write_state (fptr, 0, t + val); /* Chucking least random bit. */ *result = val >> 1; ++fptr; On most architecture that supports fast unaligned access, the resulting code won't change. However, on sparc and others, it has the drawback that the state will be updated byte-per-byte. It would be possible to speed up things by using a temporary buffer and memcpy it back, but I don't think it is worth.
Oh! That's fine then.
> I think the way to fix unaligned access without breaking the API is internally use memcpy or packed struct Nice! I wanted to suggest to test _STRING_ARCH_unaligned. But _STRING_ARCH_unaligned has already been removed — in favour of memcpy or a packed struct :) Really nice that we can rely on the compiler for such arch dependent things nowadays.
azanella, do you plan on handling it or am I? I am happy to take it.
I'll take it.
I'm just working on adding a testcase. I should be able to submit it soon and hopefully can be in glibc-2.41 (and backportable).
Patch posted: https://inbox.sourceware.org/libc-alpha/6180a8251d3f8d714ff13d27ccce48e44845661b.1733802592.git.sam@gentoo.org/
Fixed for 2.41. Let's see how things go and may backport later on commit d5bceac99d24af1131b90027dab267e437b65cd1 (HEAD -> master, upstream/master, origin/master, origin/HEAD, sam-30584) Author: Sam James <sam@gentoo.org> Date: Tue Dec 10 01:21:46 2024 +0000 stdlib: random_r: fix unaligned access in initstate and initstate_r [BZ #30584] The initstate{,_r} interfaces are documented in BSD as needing an aligned array of 32-bit values, but neither POSIX nor glibc's own documentation require it to be aligned. glibc's documentation says it "should" be a power of 2, but not must. Use memcpy to read and write to `state` to handle such an unaligned argument. Co-authored-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> Reviewed-by: Florian Weimer <fweimer@redhat.com>