mmap error handling

PATCH In review Pavel Roskin 3 participants 4 messages Jun 15, 2016

lore

Share & tools

lore

Pavel Roskin

author

2016-06-15 22:42 UTC #0 lore raw

PATCH 1/1 mmap error handling

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 <[email protected]>

diff.c +3 -1

@@ -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];

diffcore-order.c +1 -1

@@ -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++) {

local-pull.c +1 -1

@@ -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;
 		}

read-cache.c +2 -2

@@ -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;

rev-cache.c +2 -4

@@ -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;

sha1_file.c +2 -2

@@ -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)

test-delta.c +2 -2

@@ -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,

tools/mailsplit.c +2 -2

@@ -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];

Signature · 2 lines

Regards,
Pavel Roskin

Morten Welinder

2016-06-15 22:42 UTC #1 lore raw

Quoted 2 lines

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

Linus Torvalds

2016-06-15 22:42 UTC #2 lore raw

Quoted 9 lines · Morten Welinder

Quoted 3 lines
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

Pavel Roskin

author

2016-06-15 22:42 UTC #3 lore raw

Hi, Linus!

Quoted 2 lines · Linus Torvalds