{"thread":{"id":"21510","subject":"[cgit PATCH] Close file descriptor on error in readfile()","startedAt":"2009-11-07T02:01:16Z","lastAt":"2009-11-07T17:15:34Z","messageCount":7,"participants":["Rys Sommefeldt","Steven Noonan","Lars Hjemli","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"127027","messageId":"4AF4D4EC.1040806@pixeltards.com","threadId":"21510","inReplyTo":null,"subject":"[cgit PATCH] Close file descriptor on error in readfile()","fromName":"Rys Sommefeldt","fromEmail":"rys@pixeltards.com","sentAt":"2009-11-07T02:01:16Z","receivedAt":"2009-11-07T02:01:16Z","isPatch":true,"sender":{"key":"rys@pixeltards.com","avatar":"https://gravatar.com/avatar/60da3cc376692dd205d1e9c68d4c1d7904b9e1a9076f34893f6c8b86272376af?d=mp&s=160"},"body":"Hi Lars,\n\nMy colleagues and I use cgit at work, and we've found that the scanning \nprocess can consume all available fds pretty quickly on our cgit hosts, \nbecause it doesn't close them properly on error. We have a few thousand \nactive repositories for cgit to scan, and we noticed it dying after a \ncertain amount.\n\nI've attached a patch which should apply against current master, \nalthough I developed it a while back on an older 0.8 version (sorry it \ntook so long to subscribe and send the patch in).\n\nCheers,\n\nRys Sommefeldt\n---\n\n From 6446cf839d2104cd40848e439bf97cd7fd6ccfee Mon Sep 17 00:00:00 2001\nFrom: Rys Sommefeldt \nDate: Fri, 6 Nov 2009 17:14:56 +0000\nSubject: [PATCH] Close fd when done\n\n---\n shared.c | 9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/shared.c b/shared.c\nindex d7b2d5a..d5e54e6 100644\n--- a/shared.c\n+++ b/shared.c\n@@ -404,14 +404,19 @@ int readfile(const char *path, char **buf, size_t \n*size)\n struct stat st;\n \n fd = open(path, O_RDONLY);\n- if (fd == -1)\n+ if (fd == -1) {\n+ close(fd);\n return errno;\n- if (fstat(fd, &st))\n+ }\n+ if (fstat(fd, &st)) {\n+ close(fd);\n return errno;\n+ }\n if (!S_ISREG(st.st_mode))\n return EISDIR;\n *buf = xmalloc(st.st_size + 1);\n *size = read_in_full(fd, *buf, st.st_size);\n (*buf)[*size] = '\\0';\n+ close(fd);\n return (*size == st.st_size ? 0 : errno);\n }\n-- \n1.6.5.2\n"},{"id":"127028","messageId":"f488382f0911061822y7d0b52d5sa5cf4b199554312f@mail.gmail.com","threadId":"21510","inReplyTo":"4AF4D4EC.1040806@pixeltards.com","subject":"Re: [cgit PATCH] Close file descriptor on error in readfile()","fromName":"Steven Noonan","fromEmail":"steven@uplinklabs.net","sentAt":"2009-11-07T02:22:09Z","receivedAt":"2009-11-07T02:22:09Z","isPatch":true,"sender":{"key":"steven@uplinklabs.net","avatar":"https://gravatar.com/avatar/b0cd397a10638433f76e084531aa0af3bef85f8fdb59b1ebe2ddaf168cd100e9?d=mp&s=160"},"body":"On Fri, Nov 6, 2009 at 6:01 PM, Rys Sommefeldt wrote:\n> Hi Lars,\n>\n> My colleagues and I use cgit at work, and we've found that the scanning\n> process can consume all available fds pretty quickly on our cgit hosts,\n> because it doesn't close them properly on error.  We have a few thousand\n> active repositories for cgit to scan, and we noticed it dying after a\n> certain amount.\n>\n> I've attached a patch which should apply against current master, although I\n> developed it a while back on an older 0.8 version (sorry it took so long to\n> subscribe and send the patch in).\n>\n> Cheers,\n>\n> Rys Sommefeldt\n> ---\n>\n> From 6446cf839d2104cd40848e439bf97cd7fd6ccfee Mon Sep 17 00:00:00 2001\n> From: Rys Sommefeldt \n> Date: Fri, 6 Nov 2009 17:14:56 +0000\n> Subject: [PATCH] Close fd when done\n>\n> ---\n> shared.c |    9 +++++++--\n> 1 files changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/shared.c b/shared.c\n> index d7b2d5a..d5e54e6 100644\n> --- a/shared.c\n> +++ b/shared.c\n> @@ -404,14 +404,19 @@ int readfile(const char *path, char **buf, size_t\n> *size)\n>    struct stat st;\n>\n>    fd = open(path, O_RDONLY);\n> -    if (fd == -1)\n> +    if (fd == -1) {\n> +        close(fd);\n>        return errno;\n> -    if (fstat(fd, &st))\n> +    }\n\nThe above change looks bogus. If fd == -1, you close() it anyway?\n\n> +    if (fstat(fd, &st)) {\n> +        close(fd);\n>        return errno;\n> +    }\n>    if (!S_ISREG(st.st_mode))\n>        return EISDIR;\n>    *buf = xmalloc(st.st_size + 1);\n>    *size = read_in_full(fd, *buf, st.st_size);\n>    (*buf)[*size] = '\\0';\n> +    close(fd);\n>    return (*size == st.st_size ? 0 : errno);\n> }\n> --\n> 1.6.5.2\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"127029","messageId":"4AF4DAC9.3020805@pixeltards.com","threadId":"21510","inReplyTo":"f488382f0911061822y7d0b52d5sa5cf4b199554312f@mail.gmail.com","subject":"Re: [cgit PATCH] Close file descriptor on error in readfile()","fromName":"Rys Sommefeldt","fromEmail":"rys@pixeltards.com","sentAt":"2009-11-07T02:26:17Z","receivedAt":"2009-11-07T02:26:17Z","isPatch":true,"sender":{"key":"rys@pixeltards.com","avatar":"https://gravatar.com/avatar/60da3cc376692dd205d1e9c68d4c1d7904b9e1a9076f34893f6c8b86272376af?d=mp&s=160"},"body":"Steven Noonan wrote:\n> The above change looks bogus. If fd == -1, you close() it anyway?\n> \nAh, of course, sorry. I'll redo the patch.\n>> + if (fstat(fd, &st)) {\n>> + close(fd);\n>> return errno;\n>> + }\n>> if (!S_ISREG(st.st_mode))\n>> return EISDIR;\n>> *buf = xmalloc(st.st_size + 1);\n>> *size = read_in_full(fd, *buf, st.st_size);\n>> (*buf)[*size] = '\\0';\n>> + close(fd);\n>> return (*size == st.st_size ? 0 : errno);\n>> }\n>> --\n>> 1.6.5.2\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n>>\n>> \n> --\n> \n\n\n__________ Information from ESET NOD32 Antivirus, version of virus signature database 4580 (20091106) __________\n\nThe message was checked by ESET NOD32 Antivirus.\n\nhttp://www.eset.com\n"},{"id":"127039","messageId":"4AF566C9.5090106@pixeltards.com","threadId":"21510","inReplyTo":"4AF4D4EC.1040806@pixeltards.com","subject":"Re: [cgit PATCH] Close file descriptor on error in readfile()","fromName":"Rys Sommefeldt","fromEmail":"rys@pixeltards.com","sentAt":"2009-11-07T12:23:37Z","receivedAt":"2009-11-07T12:23:37Z","isPatch":true,"sender":{"key":"rys@pixeltards.com","avatar":"https://gravatar.com/avatar/60da3cc376692dd205d1e9c68d4c1d7904b9e1a9076f34893f6c8b86272376af?d=mp&s=160"},"body":"All,\n\nSorry for the earlier HTML email, I'd misconfigured my mail client so \naccept my apologies for that (and thanks Steven). Here's the reworked \npatch:\n\n From d928507bf4c8727c3848525f4744d7c8507de5e8 Mon Sep 17 00:00:00 2001\nFrom: Rys Sommefeldt \nDate: Sat, 7 Nov 2009 12:15:24 +0000\nSubject: [PATCH] Close fd on error in readfile()\n\n---\n shared.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/shared.c b/shared.c\nindex d7b2d5a..a676fa3 100644\n--- a/shared.c\n+++ b/shared.c\n@@ -406,12 +406,15 @@ int readfile(const char *path, char **buf, size_t \n*size)\n fd = open(path, O_RDONLY);\n if (fd == -1)\n return errno;\n- if (fstat(fd, &st))\n+ if (fstat(fd, &st)) {\n+ close(fd);\n return errno;\n+ }\n if (!S_ISREG(st.st_mode))\n return EISDIR;\n *buf = xmalloc(st.st_size + 1);\n *size = read_in_full(fd, *buf, st.st_size);\n (*buf)[*size] = '\\0';\n+ close(fd);\n return (*size == st.st_size ? 0 : errno);\n }\n-- \n1.6.5.2\n"},{"id":"127043","messageId":"8c5c35580911070659h35c44421q713ddba97318e2b8@mail.gmail.com","threadId":"21510","inReplyTo":"4AF566C9.5090106@pixeltards.com","subject":"Re: [cgit PATCH] Close file descriptor on error in readfile()","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-11-07T14:59:00Z","receivedAt":"2009-11-07T14:59:00Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Sat, Nov 7, 2009 at 13:23, Rys Sommefeldt wrote:\n> Sorry for the earlier HTML email, I'd misconfigured my mail client so accept\n> my apologies for that (and thanks Steven).  Here's the reworked patch:\n\nThanks. I've applied the following to my stable branch:\n\ndiff --git a/shared.c b/shared.c\nindex d7b2d5a..a27ab30 100644\n--- a/shared.c\n+++ b/shared.c\n@@ -406,12 +406,17 @@ int readfile(const char *path, char **buf, size_t *size)\n fd = open(path, O_RDONLY);\n if (fd == -1)\n return errno;\n- if (fstat(fd, &st))\n+ if (fstat(fd, &st)) {\n+ close(fd);\n return errno;\n- if (!S_ISREG(st.st_mode))\n+ }\n+ if (!S_ISREG(st.st_mode)) {\n+ close(fd);\n return EISDIR;\n+ }\n *buf = xmalloc(st.st_size + 1);\n *size = read_in_full(fd, *buf, st.st_size);\n (*buf)[*size] = '\\0';\n+ close(fd);\n return (*size == st.st_size ? 0 : errno);\n }\n\n--\nlarsh\n"},{"id":"129237","messageId":"m2ocneb9cc.fsf@igel.home","threadId":"21510","inReplyTo":"8c5c35580911070659h35c44421q713ddba97318e2b8@mail.gmail.com","subject":"Re: [cgit PATCH] Close file descriptor on error in readfile()","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2009-11-07T16:14:43Z","receivedAt":"2009-11-07T16:14:43Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Lars Hjemli writes:\n\n> diff --git a/shared.c b/shared.c\n> index d7b2d5a..a27ab30 100644\n> --- a/shared.c\n> +++ b/shared.c\n> @@ -406,12 +406,17 @@ int readfile(const char *path, char **buf, size_t *size)\n> fd = open(path, O_RDONLY);\n> if (fd == -1)\n> return errno;\n> - if (fstat(fd, &st))\n> + if (fstat(fd, &st)) {\n> + close(fd);\n> return errno;\n\nThe close call can clobber errno.\n\n> - if (!S_ISREG(st.st_mode))\n> + }\n> + if (!S_ISREG(st.st_mode)) {\n> + close(fd);\n> return EISDIR;\n> + }\n> *buf = xmalloc(st.st_size + 1);\n> *size = read_in_full(fd, *buf, st.st_size);\n> (*buf)[*size] = '\\0';\n> + close(fd);\n> return (*size == st.st_size ? 0 : errno);\n\nLikewise.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"127050","messageId":"8c5c35580911070915j7a0100f5sc666b3294bca3941@mail.gmail.com","threadId":"21510","inReplyTo":"m2ocneb9cc.fsf@igel.home","subject":"Re: [cgit PATCH] Close file descriptor on error in readfile()","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-11-07T17:15:34Z","receivedAt":"2009-11-07T17:15:34Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Sat, Nov 7, 2009 at 17:14, Andreas Schwab wrote:\n> Lars Hjemli writes:\n>\n>> diff --git a/shared.c b/shared.c\n>> index d7b2d5a..a27ab30 100644\n>> --- a/shared.c\n>> +++ b/shared.c\n>> @@ -406,12 +406,17 @@ int readfile(const char *path, char **buf, size_t *size)\n>>         fd = open(path, O_RDONLY);\n>>         if (fd == -1)\n>>                 return errno;\n>> -       if (fstat(fd, &st))\n>> +       if (fstat(fd, &st)) {\n>> +               close(fd);\n>>                 return errno;\n>\n> The close call can clobber errno.\n>\n>> -       if (!S_ISREG(st.st_mode))\n>> +       }\n>> +       if (!S_ISREG(st.st_mode)) {\n>> +               close(fd);\n>>                 return EISDIR;\n>> +       }\n>>         *buf = xmalloc(st.st_size + 1);\n>>         *size = read_in_full(fd, *buf, st.st_size);\n>>         (*buf)[*size] = '\\0';\n>> +       close(fd);\n>>         return (*size == st.st_size ? 0 : errno);\n>\n> Likewise.\n\nThanks for noticing. I've applied the following patch on top of the bad one:\n\nFrom 21f67e7d82986135922aece6b4ebf410a98705bc Mon Sep 17 00:00:00 2001\nFrom: Lars Hjemli \nDate: Sat, 7 Nov 2009 18:08:30 +0100\nSubject: [PATCH] shared.c: return original errno\n\nNoticed-by: Andreas Schwab \nSigned-off-by: Lars Hjemli \n---\n shared.c | 8 +++++---\n 1 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/shared.c b/shared.c\nindex a27ab30..9362d21 100644\n--- a/shared.c\n+++ b/shared.c\n@@ -400,15 +400,16 @@ int cgit_close_filter(struct cgit_filter *filter)\n */\n int readfile(const char *path, char **buf, size_t *size)\n {\n- int fd;\n+ int fd, e;\n struct stat st;\n\n fd = open(path, O_RDONLY);\n if (fd == -1)\n return errno;\n if (fstat(fd, &st)) {\n+ e = errno;\n close(fd);\n- return errno;\n+ return e;\n }\n if (!S_ISREG(st.st_mode)) {\n close(fd);\n@@ -416,7 +417,8 @@ int readfile(const char *path, char **buf, size_t *size)\n }\n *buf = xmalloc(st.st_size + 1);\n *size = read_in_full(fd, *buf, st.st_size);\n+ e = errno;\n (*buf)[*size] = '\\0';\n close(fd);\n- return (*size == st.st_size ? 0 : errno);\n+ return (*size == st.st_size ? 0 : e);\n }\n\n-- \nlarsh\n"}]}