Make _init_rand() thread safe - use rand_r() instead of rand().

PATCH New Dave Wysochanski 2 participants 5 messages Dec 5, 2008 – Dec 6, 2008

Share & tools

Dave Wysochanski

author

2008-12-05 22:52 UTC #0 lore raw

PATCH 1/1 Make _init_rand() thread safe - use rand_r() instead of rand().

This random code is for temp files. Other code which uses /dev/urandom is used for uuids. Should we be using /dev/urandom everywhere?

Signed-off-by Dave Wysochanski <[email protected]>

---
 lib/commands/toolcontext.c    |    7 +++++++
 lib/commands/toolcontext.h    |    1 +
 lib/format_text/archive.c     |    3 ++-
 lib/format_text/format-text.c |    3 ++-
 lib/misc/lvm-file.c           |    5 +++--
 lib/misc/lvm-file.h           |    3 ++-
 tools/lvmcmdline.c            |    7 -------
 7 files changed, 17 insertions(+), 12 deletions(-)

lib/commands/toolcontext.c +7 -0

index f49c7c6..5eb1039 100644
@@ -979,6 +979,11 @@ static int _init_backup(struct cmd_context *cmd)
 	return 1;
 }
 
+static void _init_rand(struct cmd_context *cmd)
+{
+	cmd->rand_seed = (unsigned) time(NULL) + (unsigned) getpid();
+}
+
 /* Entry point */
 struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
 				       unsigned is_long_lived)

@@ -1077,6 +1082,8 @@ struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
 	if (!_init_backup(cmd))
 		goto error;
 
+	_init_rand(cmd);
+
 	cmd->default_settings.cache_vgmetadata = 1;
 	cmd->current_settings = cmd->default_settings;

lib/commands/toolcontext.h +1 -0

index 6cbf6a6..4ea3486 100644
@@ -62,6 +62,7 @@ struct cmd_context {
 	const char *hostname;
 	const char *kernel_vsn;
 
+	unsigned rand_seed;
 	char *cmd_line;
 	struct command *command;
 	struct arg *args;

lib/format_text/archive.c +2 -1

index b232db7..3e5b0c4 100644
@@ -236,7 +236,8 @@ int archive_vg(struct volume_group *vg,
 	/*
 	 * Write the vg out to a temporary file.
 	 */
-	if (!create_temp_name(dir, temp_file, sizeof(temp_file), &fd)) {
+	if (!create_temp_name(dir, temp_file, sizeof(temp_file), &fd,
+			      &vg->cmd->rand_seed)) {
 		log_err("Couldn't create temporary archive name.");
 		return 0;
 	}

lib/format_text/format-text.c +2 -1

index fc4e5ff..af9c2ac 100644
@@ -848,7 +848,8 @@ static int _vg_write_file(struct format_instance *fid __attribute((unused)),
 		return 0;
 	}
 
-	if (!create_temp_name(temp_dir, temp_file, sizeof(temp_file), &fd)) {
+	if (!create_temp_name(temp_dir, temp_file, sizeof(temp_file), &fd,
+			      &vg->cmd->rand_seed)) {
 		log_err("Couldn't create temporary text file name.");
 		return 0;
 	}

lib/misc/lvm-file.c +3 -2

index 3e812d1..a5a5f2b 100644
@@ -29,7 +29,8 @@
  * rename the file after successfully writing it.  Grab
  * NFS-supported exclusive fcntl discretionary lock.
  */
-int create_temp_name(const char *dir, char *buffer, size_t len, int *fd)
+int create_temp_name(const char *dir, char *buffer, size_t len, int *fd,
+		     unsigned *seed)
 {
 	int i, num;
 	pid_t pid;

@@ -41,7 +42,7 @@ int create_temp_name(const char *dir, char *buffer, size_t len, int *fd)
 		.l_len = 0
 	};
 
-	num = rand();
+	num = rand_r(seed);
 	pid = getpid();
 	if (gethostname(hostname, sizeof(hostname)) < 0) {
 		log_sys_error("gethostname", "");

lib/misc/lvm-file.h +2 -1

index 8110be1..07327c3 100644
@@ -19,7 +19,8 @@
 /*
  * Create a temporary filename, and opens a descriptor to the file.
  */
-int create_temp_name(const char *dir, char *buffer, size_t len, int *fd);
+int create_temp_name(const char *dir, char *buffer, size_t len, int *fd,
+		     unsigned *seed);
 
 /*
  * NFS-safe rename of a temporary file to a common name, designed

tools/lvmcmdline.c +0 -7

index 78e9e60..29b4c15 100644
@@ -997,11 +997,6 @@ int lvm_split(char *str, int *argc, char **argv, int max)
 	return *argc;
 }
 
-static void _init_rand(void)
-{
-	srand((unsigned) time(NULL) + (unsigned) getpid());
-}
-
 static const char *_get_cmdline(pid_t pid)
 {
 	static char _proc_cmdline[32];

@@ -1096,8 +1091,6 @@ struct cmd_context *init_lvm(unsigned is_static)
 	if (!(cmd = create_toolcontext(_cmdline.the_args, is_static, 0)))
 		return_NULL;
 
-	_init_rand();
-
 	_apply_settings(cmd);
 
 	return cmd;

Signature · 1 line

1.5.5.1

Alasdair G Kergon

2008-12-06 02:26 UTC #1 lore raw

PATCH 1/1 Make _init_rand() thread safe - use rand_r() instead of rand().

Quoted 3 lines · Dave Wysochanski

This random code is for temp files. Other code which uses /dev/urandom is used for uuids. Should we be using /dev/urandom everywhere?

Well look what we are using it for - we are not relying on the randomness
- if the filename is already in use, we just try another one - so it's nothing to worry about.

lib/format_text/format-text.c static int _vg_write_file(struct format_instance *fid __attribute((unused)), · line 852

Quoted 2 lines

lib/format_text/format-text.c +2 -0

+	if (!create_temp_name(temp_dir, temp_file, sizeof(temp_file), &fd,
+			      &vg->cmd->rand_seed)) {

Just pass 'cmd'? (as first arg)

Does this patch do enough to ensure each thread will get a different value of 'seed'?

Alasdair

Signature · 1 line

agk at redhat.com

Dave Wysochanski

author

2008-12-06 03:27 UTC #2 lore raw

PATCH 1/1 Make _init_rand() thread safe - use rand_r() instead of rand().

Quoted 5 lines · Alasdair G Kergon

Quoted 3 lines · Dave Wysochanski
This random code is for temp files. Other code which uses /dev/urandom is used for uuids. Should we be using /dev/urandom everywhere?

Quoted 9 lines

Well look what we are using it for - we are not relying on the randomness
- if the filename is already in use, we just try another one - so it's nothing to worry about.
lib/format_text/format-text.c static int _vg_write_file(struct format_instance *fid __attribute((unused)), · line 852
Quoted 2 lines
lib/format_text/format-text.c +2 -0
+	if (!create_temp_name(temp_dir, temp_file, sizeof(temp_file), &fd,
+			      &vg->cmd->rand_seed)) {
Just pass 'cmd'? (as first arg)

I originally did that but then thought it might be better to avoid polluting lvm-file.c with includes just so I could dereference cmd.

Quoted 3 lines

Does this patch do enough to ensure each thread will get a different value of 'seed'?

Probably not. A better seed might use /dev/urandom or gettimeofday() with tv_usec granularity.

Alasdair G Kergon

2008-12-06 13:58 UTC #3 lore raw

PATCH 1/1 Make _init_rand() thread safe - use rand_r() instead of rand().

Quoted 9 lines · Dave Wysochanski

Quoted 5 lines · Alasdair G Kergon
lib/format_text/format-text.c static int _vg_write_file(struct format_instance *fid __attribute((unused)), · line 852
Quoted 2 lines · Dave Wysochanski
lib/format_text/format-text.c +2 -0
+	if (!create_temp_name(temp_dir, temp_file, sizeof(temp_file), &fd,
+			      &vg->cmd->rand_seed)) {
Just pass 'cmd'? (as first arg)
I originally did that but then thought it might be better to avoid polluting lvm-file.c with includes just so I could dereference cmd.

Doesn't really matter as lvm-file.c is within the library, and the include you need is also internal. Not important though, don't bother changing it now if you don't want to.

Quoted 4 lines

Quoted 2 lines
Does this patch do enough to ensure each thread will get a different value of 'seed'?
Probably not. A better seed might use /dev/urandom or gettimeofday() with tv_usec granularity.

Yup - the existing one-second granularity makes a collision likely - tv_usec ought to be adequate.

Alasdair

Signature · 1 line

agk at redhat.com

Dave Wysochanski

author

2008-12-06 16:16 UTC #4 lore raw

PATCH 1/1 Make _init_rand() thread safe - use rand_r() instead of rand().

Quoted 9 lines · Alasdair G Kergon

Quoted 5 lines
Quoted 3 lines
Does this patch do enough to ensure each thread will get a different value of 'seed'?
Probably not. A better seed might use /dev/urandom or gettimeofday() with tv_usec granularity.
Yup - the existing one-second granularity makes a collision likely - tv_usec ought to be adequate.

Did not see this before I started adding the generic read_urandom() function.