{"thread":{"id":"1358","subject":"[PATCH] mmap error handling","startedAt":"2005-07-28T21:40:42Z","lastAt":"2005-07-29T14:49:14Z","messageCount":4,"participants":["Pavel Roskin","Morten Welinder","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"6534","messageId":"1122586842.17837.9.camel@dv","threadId":"1358","inReplyTo":null,"subject":"[PATCH] mmap error handling","fromName":"Pavel Roskin","fromEmail":"proski@gnu.org","sentAt":"2005-07-28T21:40:42Z","receivedAt":"2005-07-28T21:40:42Z","isPatch":true,"sender":{"key":"proski@gnu.org","avatar":null},"body":"Hello!\n\nI have reviewed all occurrences of mmap() in git and fixed three types\nof errors/defects:\n\n1) The result is not checked.\n2) The file descriptor is closed if mmap() succeeds, but not when it\nfails.\n3) Various casts applied to -1 are used instead of MAP_FAILED, which is\nspecifically defined to check mmap() return value.\n\nI have verified that successful close() after failed mmap() won't reset\nthe output of perror() to \"Success\".\n\nSigned-off-by: Pavel Roskin <proski@gnu.org>\n\ndiff --git a/diff.c b/diff.c\n--- a/diff.c\n+++ b/diff.c\n@@ -377,8 +377,10 @@ int diff_populate_filespec(struct diff_f\n \t\tif (fd < 0)\n \t\t\tgoto err_empty;\n \t\ts->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\ts->should_munmap = 1;\n \t\tclose(fd);\n+\t\tif (s->data == MAP_FAILED)\n+\t\t\tgoto err_empty;\n+\t\ts->should_munmap = 1;\n \t}\n \telse {\n \t\tchar type[20];\ndiff --git a/diffcore-order.c b/diffcore-order.c\n--- a/diffcore-order.c\n+++ b/diffcore-order.c\n@@ -28,7 +28,7 @@ static void prepare_order(const char *or\n \t}\n \tmap = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (-1 == (int)(long)map)\n+\tif (map == MAP_FAILED)\n \t\treturn;\n \tendp = map + st.st_size;\n \tfor (pass = 0; pass < 2; pass++) {\ndiff --git a/local-pull.c b/local-pull.c\n--- a/local-pull.c\n+++ b/local-pull.c\n@@ -54,7 +54,7 @@ int fetch(unsigned char *sha1)\n \t\t}\n \t\tmap = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, ifd, 0);\n \t\tclose(ifd);\n-\t\tif (-1 == (int)(long)map) {\n+\t\tif (map == MAP_FAILED) {\n \t\t\tfprintf(stderr, \"cannot mmap %s\\n\", filename);\n \t\t\treturn -1;\n \t\t}\ndiff --git a/read-cache.c b/read-cache.c\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -392,7 +392,7 @@ int read_cache(void)\n \t\treturn (errno == ENOENT) ? 0 : error(\"open failed\");\n \n \tsize = 0; // avoid gcc warning\n-\tmap = (void *)-1;\n+\tmap = MAP_FAILED;\n \tif (!fstat(fd, &st)) {\n \t\tsize = st.st_size;\n \t\terrno = EINVAL;\n@@ -400,7 +400,7 @@ int read_cache(void)\n \t\t\tmap = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n \t}\n \tclose(fd);\n-\tif (-1 == (int)(long)map)\n+\tif (map == MAP_FAILED)\n \t\treturn error(\"mmap failed\");\n \n \thdr = map;\ndiff --git a/rev-cache.c b/rev-cache.c\n--- a/rev-cache.c\n+++ b/rev-cache.c\n@@ -212,11 +212,9 @@ int read_rev_cache(const char *path, FIL\n \t\treturn -1;\n \t}\n \tmap = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tif (map == MAP_FAILED) {\n-\t\tclose(fd);\n-\t\treturn -1;\n-\t}\n \tclose(fd);\n+\tif (map == MAP_FAILED)\n+\t\treturn -1;\n \n \tmemset(last_sha1, 0, 20);\n \tofs = 0;\ndiff --git a/sha1_file.c b/sha1_file.c\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -518,7 +518,7 @@ static void *map_sha1_file_internal(cons\n \t}\n \tmap = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (-1 == (int)(long)map)\n+\tif (map == MAP_FAILED)\n \t\treturn NULL;\n \t*size = st.st_size;\n \treturn map;\n@@ -1363,7 +1363,7 @@ int index_fd(unsigned char *sha1, int fd\n \tif (size)\n \t\tbuf = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif ((int)(long)buf == -1)\n+\tif (buf == MAP_FAILED)\n \t\treturn -1;\n \n \tif (!type)\ndiff --git a/test-delta.c b/test-delta.c\n--- a/test-delta.c\n+++ b/test-delta.c\n@@ -39,11 +39,11 @@ int main(int argc, char *argv[])\n \t}\n \tfrom_size = st.st_size;\n \tfrom_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n \tif (from_buf == MAP_FAILED) {\n \t\tperror(argv[2]);\n \t\treturn 1;\n \t}\n-\tclose(fd);\n \n \tfd = open(argv[3], O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st)) {\n@@ -52,11 +52,11 @@ int main(int argc, char *argv[])\n \t}\n \tdata_size = st.st_size;\n \tdata_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n \tif (data_buf == MAP_FAILED) {\n \t\tperror(argv[3]);\n \t\treturn 1;\n \t}\n-\tclose(fd);\n \n \tif (argv[1][1] == 'd')\n \t\tout_buf = diff_delta(from_buf, from_size,\ndiff --git a/tools/mailsplit.c b/tools/mailsplit.c\n--- a/tools/mailsplit.c\n+++ b/tools/mailsplit.c\n@@ -116,11 +116,11 @@ int main(int argc, char **argv)\n \t}\n \tsize = st.st_size;\n \tmap = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tif (-1 == (int)(long)map) {\n+\tclose(fd);\n+\tif (map == MAP_FAILED) {\n \t\tperror(\"mmap\");\n \t\texit(1);\n \t}\n-\tclose(fd);\n \tnr = 0;\n \tdo {\n \t\tchar name[10];\n\n\n-- \nRegards,\nPavel Roskin\n"},{"id":"6535","messageId":"118833cc05072816294f80a829@mail.gmail.com","threadId":"1358","inReplyTo":"1122586842.17837.9.camel@dv","subject":"Re: [PATCH] mmap error handling","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2005-07-28T23:29:57Z","receivedAt":"2005-07-28T23:29:57Z","isPatch":true,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":"> I have verified that successful close() after failed mmap() won't reset\n> the output of perror() to \"Success\".\n\nDoes $standard guarantee that?\n\nIn general, successful libc calls can set errno to whatever they\nplease, except zero.  And they sometimes do.  This follows from\nC99.\n\nMorten\n"},{"id":"6536","messageId":"Pine.LNX.4.58.0507281725440.3307@g5.osdl.org","threadId":"1358","inReplyTo":"118833cc05072816294f80a829@mail.gmail.com","subject":"Re: [PATCH] mmap error handling","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-07-29T00:30:29Z","receivedAt":"2005-07-29T00:30:29Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 28 Jul 2005, Morten Welinder wrote:\n>\n> > I have verified that successful close() after failed mmap() won't reset\n> > the output of perror() to \"Success\".\n> \n> Does $standard guarantee that?\n> \n> In general, successful libc calls can set errno to whatever they\n> please, except zero.  And they sometimes do.  This follows from\n> C99.\n\nIndeed. \n\n_always_ save the value of errno before doing any other calls. Even \nsuccessful calls are perfectly allowed to change errno.\n\n\"close()\" may not _normally_ change errno, but the fact is, not only can \nclose sometimes return an error, but it could validly have some debugging \nwrapper that does logging, and change errno because of that.\n\nYeah, we'd be better off if UNIX had always used the linux kernel practice\nof hiding errno as a negative return value (and the \"IS_ERR()\" test for\npointers), but hey, crud happens to the best of us.\n\n\t\tLinus\n"},{"id":"6592","messageId":"1122648554.3331.2.camel@dv","threadId":"1358","inReplyTo":"Pine.LNX.4.58.0507281725440.3307@g5.osdl.org","subject":"Re: [PATCH] mmap error handling","fromName":"Pavel Roskin","fromEmail":"proski@gnu.org","sentAt":"2005-07-29T14:49:14Z","receivedAt":"2005-07-29T14:49:14Z","isPatch":true,"sender":{"key":"proski@gnu.org","avatar":null},"body":"Hi, Linus!\n\nOn Thu, 2005-07-28 at 17:30 -0700, Linus Torvalds wrote:\n> _always_ save the value of errno before doing any other calls. Even \n> successful calls are perfectly allowed to change errno.\n\nOK.  Fixed patch below.\n\nSigned-off-by: Pavel Roskin <proski@gnu.org>\n\ndiff --git a/diff.c b/diff.c\n--- a/diff.c\n+++ b/diff.c\n@@ -377,8 +377,10 @@ int diff_populate_filespec(struct diff_f\n \t\tif (fd < 0)\n \t\t\tgoto err_empty;\n \t\ts->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\ts->should_munmap = 1;\n \t\tclose(fd);\n+\t\tif (s->data == MAP_FAILED)\n+\t\t\tgoto err_empty;\n+\t\ts->should_munmap = 1;\n \t}\n \telse {\n \t\tchar type[20];\ndiff --git a/diffcore-order.c b/diffcore-order.c\n--- a/diffcore-order.c\n+++ b/diffcore-order.c\n@@ -28,7 +28,7 @@ static void prepare_order(const char *or\n \t}\n \tmap = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (-1 == (int)(long)map)\n+\tif (map == MAP_FAILED)\n \t\treturn;\n \tendp = map + st.st_size;\n \tfor (pass = 0; pass < 2; pass++) {\ndiff --git a/local-pull.c b/local-pull.c\n--- a/local-pull.c\n+++ b/local-pull.c\n@@ -54,7 +54,7 @@ int fetch(unsigned char *sha1)\n \t\t}\n \t\tmap = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, ifd, 0);\n \t\tclose(ifd);\n-\t\tif (-1 == (int)(long)map) {\n+\t\tif (map == MAP_FAILED) {\n \t\t\tfprintf(stderr, \"cannot mmap %s\\n\", filename);\n \t\t\treturn -1;\n \t\t}\ndiff --git a/read-cache.c b/read-cache.c\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -392,7 +392,7 @@ int read_cache(void)\n \t\treturn (errno == ENOENT) ? 0 : error(\"open failed\");\n \n \tsize = 0; // avoid gcc warning\n-\tmap = (void *)-1;\n+\tmap = MAP_FAILED;\n \tif (!fstat(fd, &st)) {\n \t\tsize = st.st_size;\n \t\terrno = EINVAL;\n@@ -400,7 +400,7 @@ int read_cache(void)\n \t\t\tmap = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n \t}\n \tclose(fd);\n-\tif (-1 == (int)(long)map)\n+\tif (map == MAP_FAILED)\n \t\treturn error(\"mmap failed\");\n \n \thdr = map;\ndiff --git a/rev-cache.c b/rev-cache.c\n--- a/rev-cache.c\n+++ b/rev-cache.c\n@@ -212,11 +212,9 @@ int read_rev_cache(const char *path, FIL\n \t\treturn -1;\n \t}\n \tmap = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tif (map == MAP_FAILED) {\n-\t\tclose(fd);\n-\t\treturn -1;\n-\t}\n \tclose(fd);\n+\tif (map == MAP_FAILED)\n+\t\treturn -1;\n \n \tmemset(last_sha1, 0, 20);\n \tofs = 0;\ndiff --git a/sha1_file.c b/sha1_file.c\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -518,7 +518,7 @@ static void *map_sha1_file_internal(cons\n \t}\n \tmap = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (-1 == (int)(long)map)\n+\tif (map == MAP_FAILED)\n \t\treturn NULL;\n \t*size = st.st_size;\n \treturn map;\n@@ -1363,7 +1363,7 @@ int index_fd(unsigned char *sha1, int fd\n \tif (size)\n \t\tbuf = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif ((int)(long)buf == -1)\n+\tif (buf == MAP_FAILED)\n \t\treturn -1;\n \n \tif (!type)\ndiff --git a/test-delta.c b/test-delta.c\n--- a/test-delta.c\n+++ b/test-delta.c\n@@ -41,6 +41,7 @@ int main(int argc, char *argv[])\n \tfrom_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tif (from_buf == MAP_FAILED) {\n \t\tperror(argv[2]);\n+\t\tclose(fd);\n \t\treturn 1;\n \t}\n \tclose(fd);\n@@ -54,6 +55,7 @@ int main(int argc, char *argv[])\n \tdata_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tif (data_buf == MAP_FAILED) {\n \t\tperror(argv[3]);\n+\t\tclose(fd);\n \t\treturn 1;\n \t}\n \tclose(fd);\ndiff --git a/tools/mailsplit.c b/tools/mailsplit.c\n--- a/tools/mailsplit.c\n+++ b/tools/mailsplit.c\n@@ -116,8 +116,9 @@ int main(int argc, char **argv)\n \t}\n \tsize = st.st_size;\n \tmap = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tif (-1 == (int)(long)map) {\n+\tif (map == MAP_FAILED) {\n \t\tperror(\"mmap\");\n+\t\tclose(fd);\n \t\texit(1);\n \t}\n \tclose(fd);\n\n\n-- \nRegards,\nPavel Roskin\n"}]}