[PATCH] perf tools: avoid possible race condition in copyfile

* [PATCH] perf tools: avoid possible race condition in copyfile
@ 2015-06-04  8:44 Milos Vyletel
  2015-06-04  9:11 ` Ingo Molnar
  0 siblings, 1 reply; 6+ messages in thread
From: Milos Vyletel @ 2015-06-04  8:44 UTC (permalink / raw)
  To: Peter Zijlstra, Paul Mackerras, Ingo Molnar,
	Arnaldo Carvalho de Melo, Jiri Olsa, Don Zickus, Andy Shevchenko,
	Milos Vyletel, Steven Rostedt (Red Hat),
	open list:PERFORMANCE EVENT...

Use unique temporary files when copying to buildid dir to prevent races
in case multiple instances are trying to copy same file. This is done by

- creating template in form <path>/.<filename>.XXXXXX where the suffix is
  used by mkstemp() to create unique file
- change file mode
- copy content
- if successful link temp file to target file
- unlink temp file

At this point the only file left at target path should be the desired
one either created by us or other instance if we raced. This should also
prevent not yet fully copied files to be visible to to other perf
instances that could try to parse them.

On top of that slow_copyfile no longer needs to deal with file mode when
creating file since temporary file is already created and mode is set.

Signed-off-by: Milos Vyletel <milos@redhat.com>
---
 tools/perf/util/util.c | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 4ee6d0d..fec1e13 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -72,20 +72,17 @@ int mkdir_p(char *path, mode_t mode)
 	return (stat(path, &st) && mkdir(path, mode)) ? -1 : 0;
 }
 
-static int slow_copyfile(const char *from, const char *to, mode_t mode)
+static int slow_copyfile(const char *from, const char *to)
 {
 	int err = -1;
 	char *line = NULL;
 	size_t n;
 	FILE *from_fp = fopen(from, "r"), *to_fp;
-	mode_t old_umask;
 
 	if (from_fp == NULL)
 		goto out;
 
-	old_umask = umask(mode ^ 0777);
 	to_fp = fopen(to, "w");
-	umask(old_umask);
 	if (to_fp == NULL)
 		goto out_fclose_from;
 
@@ -108,36 +105,55 @@ int copyfile_mode(const char *from, const char *to, mode_t mode)
 	struct stat st;
 	void *addr;
 	int err = -1;
+	char *tmp = NULL, *ptr = NULL;
 
 	if (stat(from, &st))
 		goto out;
 
-	if (st.st_size == 0) /* /proc? do it slowly... */
-		return slow_copyfile(from, to, mode);
-
-	fromfd = open(from, O_RDONLY);
-	if (fromfd < 0)
+	/* extra 'x' at the end is to reserve space for '.' */
+	if (asprintf(&tmp, "%s.XXXXXXx", to) < 0) {
+		tmp = NULL;
 		goto out;
+	}
+	ptr = strrchr(tmp, '/');
+	if (!ptr)
+		goto out;
+	ptr = memmove(ptr + 1, ptr, strlen(ptr) - 1);
+	*ptr = '.';
 
-	tofd = creat(to, mode);
+	tofd = mkstemp(tmp);
 	if (tofd < 0)
-		goto out_close_from;
+		goto out;
+
+	if (fchmod(tofd, mode))
+		goto out_close_to;
+
+	if (st.st_size == 0) { /* /proc? do it slowly... */
+		err = slow_copyfile(from, tmp);
+		goto out_close_to;
+	}
+
+	fromfd = open(from, O_RDONLY);
+	if (fromfd < 0)
+		goto out_close_to;
 
 	addr = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fromfd, 0);
 	if (addr == MAP_FAILED)
-		goto out_close_to;
+		goto out_close_from;
 
 	if (write(tofd, addr, st.st_size) == st.st_size)
 		err = 0;
 
 	munmap(addr, st.st_size);
-out_close_to:
-	close(tofd);
-	if (err)
-		unlink(to);
 out_close_from:
 	close(fromfd);
+out_close_to:
+	close(tofd);
+	if (!err)
+		err = link(tmp, to);
+	unlink(tmp);
 out:
+	free(tmp);
 	return err;
 }
 
-- 
2.4.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread