[PATCH] mmap error handling

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.

[jc: This is a second round of Pavel's patch.  He fixed up the problem
that close() potentially clobbering the errno from mmap, which
the first round had.]

Signed-off-by: Pavel Roskin <proski@gnu.org>
Signed-off-by: Junio C Hamano <junkio@cox.net>

diff --git a/diff.c b/diff.c
index 4a4b62191284a2289631c01b3a2885387f59a8c0..361d7c09ddfad2cb7e1589367ec74bad8f9ca67d 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -377,8 +377,10 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
                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
index a03862c1ce73995065bf838b408e8a6d9396025b..b38122361f837e2e2577a89ae7aec311730b7f10 100644 (file)
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -28,7 +28,7 @@ static void prepare_order(const char *orderfile)
        }
        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
index 2f06fbee8b840a7ae642f5a22e2cb993687f3470..908e18750903ddbec13f38ab8e1f389685ce9212 100644 (file)
--- 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
index f448ab17e279d2fb4e2cfa91cfc61be6f91128db..5820f18d9a79fd83ff5a418a5dfc68bbf7c5c354 100644 (file)
--- 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
index ea65274ed0cdea6291fd2820881bdb78704f319a..f908ce7a3a9883f7c10e17c19438995abc1dcfef 100644 (file)
--- a/rev-cache.c
+++ b/rev-cache.c
@@ -212,11 +212,9 @@ int read_rev_cache(const char *path, FILE *dumpfile, int dry_run)
                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
index 5ec5598d7d6cd56ebb40b21c497c0ae3db1dca57..eba5a36f246af9bbddb4558bd45834c5ef4fe71f 100644 (file)
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -518,7 +518,7 @@ static void *map_sha1_file_internal(const unsigned char *sha1,
        }
        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, struct stat *st, int write_object, con
        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
index 37ef86b283c2378017f1fc6bff05cc33fb25d7aa..e5d31ca2e7f913e461e818cb1a80ff4037296b9e 100644 (file)
--- 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
index 9379fbc5e84983e5ea0754a6587cc3490c696c69..7b712081cbf812c551be536cddbbf563c570dc75 100644 (file)
--- 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);