[PATCH] mmap error handling

[PATCH] mmap error handling

[Pavel Roskin]

Hello!

I have reviewed all occurrences of mmap() in git and fixed three types
of errors/defects:

1) The result is not checked.
2) The file descriptor is closed if mmap() succeeds, but not when it
fails.
3) Various casts applied to -1 are used instead of MAP_FAILED, which is
specifically defined to check mmap() return value.

I have verified that successful close() after failed mmap() won't reset
the output of perror() to "Success".

Signed-off-by: Pavel Roskin <proski@gnu.org>

diff --git a/diff.c b/diff.c
--- a/diff.c
+++ b/diff.c
@@ -377,8 +377,10 @@ int diff_populate_filespec(struct diff_f
 		if (fd < 0)
 			goto err_empty;
 		s->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
-		s->should_munmap = 1;
 		close(fd);
+		if (s->data == MAP_FAILED)
+			goto err_empty;
+		s->should_munmap = 1;
 	}
 	else {
 		char type[20];
diff --git a/diffcore-order.c b/diffcore-order.c
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -28,7 +28,7 @@ static void prepare_order(const char *or
 	}
 	map = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (-1 == (int)(long)map)
+	if (map == MAP_FAILED)
 		return;
 	endp = map + st.st_size;
 	for (pass = 0; pass < 2; pass++) {
diff --git a/local-pull.c b/local-pull.c
--- a/local-pull.c
+++ b/local-pull.c
@@ -54,7 +54,7 @@ int fetch(unsigned char *sha1)
 		}
 		map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, ifd, 0);
 		close(ifd);
-		if (-1 == (int)(long)map) {
+		if (map == MAP_FAILED) {
 			fprintf(stderr, "cannot mmap %s\n", filename);
 			return -1;
 		}
diff --git a/read-cache.c b/read-cache.c
--- a/read-cache.c
+++ b/read-cache.c
@@ -392,7 +392,7 @@ int read_cache(void)
 		return (errno == ENOENT) ? 0 : error("open failed");
 
 	size = 0; // avoid gcc warning
-	map = (void *)-1;
+	map = MAP_FAILED;
 	if (!fstat(fd, &st)) {
 		size = st.st_size;
 		errno = EINVAL;
@@ -400,7 +400,7 @@ int read_cache(void)
 			map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
 	}
 	close(fd);
-	if (-1 == (int)(long)map)
+	if (map == MAP_FAILED)
 		return error("mmap failed");
 
 	hdr = map;
diff --git a/rev-cache.c b/rev-cache.c
--- a/rev-cache.c
+++ b/rev-cache.c
@@ -212,11 +212,9 @@ int read_rev_cache(const char *path, FIL
 		return -1;
 	}
 	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (map == MAP_FAILED) {
-		close(fd);
-		return -1;
-	}
 	close(fd);
+	if (map == MAP_FAILED)
+		return -1;
 
 	memset(last_sha1, 0, 20);
 	ofs = 0;
diff --git a/sha1_file.c b/sha1_file.c
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -518,7 +518,7 @@ static void *map_sha1_file_internal(cons
 	}
 	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (-1 == (int)(long)map)
+	if (map == MAP_FAILED)
 		return NULL;
 	*size = st.st_size;
 	return map;
@@ -1363,7 +1363,7 @@ int index_fd(unsigned char *sha1, int fd
 	if (size)
 		buf = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if ((int)(long)buf == -1)
+	if (buf == MAP_FAILED)
 		return -1;
 
 	if (!type)
diff --git a/test-delta.c b/test-delta.c
--- a/test-delta.c
+++ b/test-delta.c
@@ -39,11 +39,11 @@ int main(int argc, char *argv[])
 	}
 	from_size = st.st_size;
 	from_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	close(fd);
 	if (from_buf == MAP_FAILED) {
 		perror(argv[2]);
 		return 1;
 	}
-	close(fd);
 
 	fd = open(argv[3], O_RDONLY);
 	if (fd < 0 || fstat(fd, &st)) {
@@ -52,11 +52,11 @@ int main(int argc, char *argv[])
 	}
 	data_size = st.st_size;
 	data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	close(fd);
 	if (data_buf == MAP_FAILED) {
 		perror(argv[3]);
 		return 1;
 	}
-	close(fd);
 
 	if (argv[1][1] == 'd')
 		out_buf = diff_delta(from_buf, from_size,
diff --git a/tools/mailsplit.c b/tools/mailsplit.c
--- a/tools/mailsplit.c
+++ b/tools/mailsplit.c
@@ -116,11 +116,11 @@ int main(int argc, char **argv)
 	}
 	size = st.st_size;
 	map = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (-1 == (int)(long)map) {
+	close(fd);
+	if (map == MAP_FAILED) {
 		perror("mmap");
 		exit(1);
 	}
-	close(fd);
 	nr = 0;
 	do {
 		char name[10];


-- 
Regards,
Pavel Roskin

Re: [PATCH] mmap error handling

[Morten Welinder]

> I have verified that successful close() after failed mmap() won't reset
> the output of perror() to "Success".

Does $standard guarantee that?

In general, successful libc calls can set errno to whatever they
please, except zero.  And they sometimes do.  This follows from
C99.

Morten

Re: [PATCH] mmap error handling

[Linus Torvalds]

On Thu, 28 Jul 2005, Morten Welinder wrote:
>
> > I have verified that successful close() after failed mmap() won't reset
> > the output of perror() to "Success".
> 
> Does $standard guarantee that?
> 
> In general, successful libc calls can set errno to whatever they
> please, except zero.  And they sometimes do.  This follows from
> C99.

Indeed. 

_always_ save the value of errno before doing any other calls. Even 
successful calls are perfectly allowed to change errno.

"close()" may not _normally_ change errno, but the fact is, not only can 
close sometimes return an error, but it could validly have some debugging 
wrapper that does logging, and change errno because of that.

Yeah, we'd be better off if UNIX had always used the linux kernel practice
of hiding errno as a negative return value (and the "IS_ERR()" test for
pointers), but hey, crud happens to the best of us.

		Linus

Re: [PATCH] mmap error handling

[Pavel Roskin]

Hi, Linus!

On Thu, 2005-07-28 at 17:30 -0700, Linus Torvalds wrote:
> _always_ save the value of errno before doing any other calls. Even 
> successful calls are perfectly allowed to change errno.

OK.  Fixed patch below.

Signed-off-by: Pavel Roskin <proski@gnu.org>

diff --git a/diff.c b/diff.c
--- a/diff.c
+++ b/diff.c
@@ -377,8 +377,10 @@ int diff_populate_filespec(struct diff_f
 		if (fd < 0)
 			goto err_empty;
 		s->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
-		s->should_munmap = 1;
 		close(fd);
+		if (s->data == MAP_FAILED)
+			goto err_empty;
+		s->should_munmap = 1;
 	}
 	else {
 		char type[20];
diff --git a/diffcore-order.c b/diffcore-order.c
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -28,7 +28,7 @@ static void prepare_order(const char *or
 	}
 	map = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (-1 == (int)(long)map)
+	if (map == MAP_FAILED)
 		return;
 	endp = map + st.st_size;
 	for (pass = 0; pass < 2; pass++) {
diff --git a/local-pull.c b/local-pull.c
--- a/local-pull.c
+++ b/local-pull.c
@@ -54,7 +54,7 @@ int fetch(unsigned char *sha1)
 		}
 		map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, ifd, 0);
 		close(ifd);
-		if (-1 == (int)(long)map) {
+		if (map == MAP_FAILED) {
 			fprintf(stderr, "cannot mmap %s\n", filename);
 			return -1;
 		}
diff --git a/read-cache.c b/read-cache.c
--- a/read-cache.c
+++ b/read-cache.c
@@ -392,7 +392,7 @@ int read_cache(void)
 		return (errno == ENOENT) ? 0 : error("open failed");
 
 	size = 0; // avoid gcc warning
-	map = (void *)-1;
+	map = MAP_FAILED;
 	if (!fstat(fd, &st)) {
 		size = st.st_size;
 		errno = EINVAL;
@@ -400,7 +400,7 @@ int read_cache(void)
 			map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
 	}
 	close(fd);
-	if (-1 == (int)(long)map)
+	if (map == MAP_FAILED)
 		return error("mmap failed");
 
 	hdr = map;
diff --git a/rev-cache.c b/rev-cache.c
--- a/rev-cache.c
+++ b/rev-cache.c
@@ -212,11 +212,9 @@ int read_rev_cache(const char *path, FIL
 		return -1;
 	}
 	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (map == MAP_FAILED) {
-		close(fd);
-		return -1;
-	}
 	close(fd);
+	if (map == MAP_FAILED)
+		return -1;
 
 	memset(last_sha1, 0, 20);
 	ofs = 0;
diff --git a/sha1_file.c b/sha1_file.c
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -518,7 +518,7 @@ static void *map_sha1_file_internal(cons
 	}
 	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (-1 == (int)(long)map)
+	if (map == MAP_FAILED)
 		return NULL;
 	*size = st.st_size;
 	return map;
@@ -1363,7 +1363,7 @@ int index_fd(unsigned char *sha1, int fd
 	if (size)
 		buf = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if ((int)(long)buf == -1)
+	if (buf == MAP_FAILED)
 		return -1;
 
 	if (!type)
diff --git a/test-delta.c b/test-delta.c
--- a/test-delta.c
+++ b/test-delta.c
@@ -41,6 +41,7 @@ int main(int argc, char *argv[])
 	from_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	if (from_buf == MAP_FAILED) {
 		perror(argv[2]);
+		close(fd);
 		return 1;
 	}
 	close(fd);
@@ -54,6 +55,7 @@ int main(int argc, char *argv[])
 	data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	if (data_buf == MAP_FAILED) {
 		perror(argv[3]);
+		close(fd);
 		return 1;
 	}
 	close(fd);
diff --git a/tools/mailsplit.c b/tools/mailsplit.c
--- a/tools/mailsplit.c
+++ b/tools/mailsplit.c
@@ -116,8 +116,9 @@ int main(int argc, char **argv)
 	}
 	size = st.st_size;
 	map = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (-1 == (int)(long)map) {
+	if (map == MAP_FAILED) {
 		perror("mmap");
+		close(fd);
 		exit(1);
 	}
 	close(fd);


-- 
Regards,
Pavel Roskin