Message ID | 1412843785-960-2-git-send-email-reftel@spotify.com |
---|---|
State | New |
Headers | show |
On 10/09/2014 02:36 AM, Magnus Reftel wrote: > This patch introduces the -seed command line option and the > QEMU_RAND_SEED environment variable for setting the random seed, which > is used for the AT_RANDOM ELF aux entry. > > Signed-off-by: Magnus Reftel <reftel@spotify.com> > --- > > +static void handle_arg_randseed(const char *arg) > +{ > + unsigned long seed; > + char* end; Style: we prefer: char *end; > + seed = strtoul(arg, &end, 0); > + if (end==arg || *end!='\0' || seed > UINT_MAX) { Style: spaces around operators: if (end == arg || *end || seed > UINT_MAX) { Bug: strtoul() sometimes reports error via errno; the only safe way to use it is to first prime errno = 0, then do strtoul, then check if errno was changed. Reimplementation: util/cutils.c already provides parse_uint() that takes care of calling strtoul safely (hmm, that version only parses 64-bit numbers; maybe we should expand it to also parse 32-bit numbers?) Surprising behavior: your code behaves differently on 32-bit hosts than it does on 64-bit hosts. Seriously. strotoul() has the annoying specification of requiring twos-complement wraparound according to the size of long, which means "-1" on a 32-bit platform parses as 0xffffffff (accepted), while on a 64-bit platform parses it as 0xffffffffffffffff (which you reject as > UINT_MAX); conversely "-18446744073709551615" fails to parse due to overflow on a 32-bit platform, while successfully being parsed as 1 on 64-bit. > + fprintf(stderr, "Invalid seed number: %s\n", arg); > + exit(1); > + } > + srand(seed); > +} > + > static void handle_arg_gdb(const char *arg) > { > gdbstub_port = atoi(arg); > @@ -3674,6 +3686,8 @@ static const struct qemu_argument arg_table[] = { > "", "run in singlestep mode"}, > {"strace", "QEMU_STRACE", false, handle_arg_strace, > "", "log system calls"}, > + {"seed", "QEMU_RAND_SEED", true, handle_arg_randseed, > + "", "Seed for pseudo-random number generator"}, > {"version", "QEMU_VERSION", false, handle_arg_version, > "", "display version information and exit"}, > {NULL, NULL, false, NULL, NULL, NULL} > @@ -3856,6 +3870,8 @@ int main(int argc, char **argv, char **envp) > cpudef_setup(); /* parse cpu definitions in target config file (TBD) */ > #endif > > + srand(time(NULL)); > + > optind = parse_args(argc, argv); > > /* Zero out regs */ > @@ -3926,6 +3942,10 @@ int main(int argc, char **argv, char **envp) > do_strace = 1; > } > > + if (getenv("QEMU_RAND_SEED")) { > + handle_arg_randseed(getenv("QEMU_RAND_SEED")); > + } Now that you have exactly one caller of the static function, it might make sense to just inline the body of that function here. > + > target_environ = envlist_to_environ(envlist, NULL); > envlist_free(envlist); > >
Hi, Thank you for your patience! I will send a third version. On Thu, Oct 9, 2014 at 5:27 PM, Eric Blake <eblake@redhat.com> wrote: > On 10/09/2014 02:36 AM, Magnus Reftel wrote: >> + char* end; > Style: we prefer: > char *end; Done. >> + if (end==arg || *end!='\0' || seed > UINT_MAX) { > Style: spaces around operators: Done. > Bug: strtoul() sometimes reports error via errno; the only safe way to > use it is to first prime errno = 0, then do strtoul, then check if errno > was changed. > > Reimplementation: util/cutils.c already provides parse_uint() that takes > care of calling strtoul safely (hmm, that version only parses 64-bit > numbers; maybe we should expand it to also parse 32-bit numbers?) Solved both by switching to parse_uint. >> + {"seed", "QEMU_RAND_SEED", true, handle_arg_randseed, >> + "", "Seed for pseudo-random number generator"}, ... >> + if (getenv("QEMU_RAND_SEED")) { >> + handle_arg_randseed(getenv("QEMU_RAND_SEED")); >> + } > > Now that you have exactly one caller of the static function, it might > make sense to just inline the body of that function here. No, it may be called using the function pointer in the argument table, above. Best Regards Magnus reftel
diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 1c04fcf..f2e2197 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -1539,7 +1539,6 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc, * Generate 16 random bytes for userspace PRNG seeding (not * cryptically secure but it's not the aim of QEMU). */ - srand((unsigned int) time(NULL)); for (i = 0; i < 16; i++) { k_rand_bytes[i] = rand(); } diff --git a/linux-user/main.c b/linux-user/main.c index 483eb3f..e80255c 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -3546,6 +3546,18 @@ static void handle_arg_pagesize(const char *arg) } } +static void handle_arg_randseed(const char *arg) +{ + unsigned long seed; + char* end; + seed = strtoul(arg, &end, 0); + if (end==arg || *end!='\0' || seed > UINT_MAX) { + fprintf(stderr, "Invalid seed number: %s\n", arg); + exit(1); + } + srand(seed); +} + static void handle_arg_gdb(const char *arg) { gdbstub_port = atoi(arg); @@ -3674,6 +3686,8 @@ static const struct qemu_argument arg_table[] = { "", "run in singlestep mode"}, {"strace", "QEMU_STRACE", false, handle_arg_strace, "", "log system calls"}, + {"seed", "QEMU_RAND_SEED", true, handle_arg_randseed, + "", "Seed for pseudo-random number generator"}, {"version", "QEMU_VERSION", false, handle_arg_version, "", "display version information and exit"}, {NULL, NULL, false, NULL, NULL, NULL} @@ -3856,6 +3870,8 @@ int main(int argc, char **argv, char **envp) cpudef_setup(); /* parse cpu definitions in target config file (TBD) */ #endif + srand(time(NULL)); + optind = parse_args(argc, argv); /* Zero out regs */ @@ -3926,6 +3942,10 @@ int main(int argc, char **argv, char **envp) do_strace = 1; } + if (getenv("QEMU_RAND_SEED")) { + handle_arg_randseed(getenv("QEMU_RAND_SEED")); + } + target_environ = envlist_to_environ(envlist, NULL); envlist_free(envlist);
This patch introduces the -seed command line option and the QEMU_RAND_SEED environment variable for setting the random seed, which is used for the AT_RANDOM ELF aux entry. Signed-off-by: Magnus Reftel <reftel@spotify.com> --- linux-user/elfload.c | 1 - linux-user/main.c | 20 ++++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)