[cgit PATCH] Close file descriptor on error in readfile()

[cgit PATCH] Close file descriptor on error in readfile()

[Rys Sommefeldt]

Hi Lars,

My colleagues and I use cgit at work, and we've found that the scanning 
process can consume all available fds pretty quickly on our cgit hosts, 
because it doesn't close them properly on error.  We have a few thousand 
active repositories for cgit to scan, and we noticed it dying after a 
certain amount.

I've attached a patch which should apply against current master, 
although I developed it a while back on an older 0.8 version (sorry it 
took so long to subscribe and send the patch in).

Cheers,

Rys Sommefeldt
---

 From 6446cf839d2104cd40848e439bf97cd7fd6ccfee Mon Sep 17 00:00:00 2001
From: Rys Sommefeldt <rsommefeldt@plus.net>
Date: Fri, 6 Nov 2009 17:14:56 +0000
Subject: [PATCH] Close fd when done

---
 shared.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/shared.c b/shared.c
index d7b2d5a..d5e54e6 100644
--- a/shared.c
+++ b/shared.c
@@ -404,14 +404,19 @@ int readfile(const char *path, char **buf, size_t 
*size)
     struct stat st;
 
     fd = open(path, O_RDONLY);
-    if (fd == -1)
+    if (fd == -1) {
+        close(fd);
         return errno;
-    if (fstat(fd, &st))
+    }
+    if (fstat(fd, &st)) {
+        close(fd);
         return errno;
+    }
     if (!S_ISREG(st.st_mode))
         return EISDIR;
     *buf = xmalloc(st.st_size + 1);
     *size = read_in_full(fd, *buf, st.st_size);
     (*buf)[*size] = '\0';
+    close(fd);
     return (*size == st.st_size ? 0 : errno);
 }
-- 
1.6.5.2

Re: [cgit PATCH] Close file descriptor on error in readfile()

[Steven Noonan]

On Fri, Nov 6, 2009 at 6:01 PM, Rys Sommefeldt <rys@pixeltards.com> wrote:
> Hi Lars,
>
> My colleagues and I use cgit at work, and we've found that the scanning
> process can consume all available fds pretty quickly on our cgit hosts,
> because it doesn't close them properly on error.  We have a few thousand
> active repositories for cgit to scan, and we noticed it dying after a
> certain amount.
>
> I've attached a patch which should apply against current master, although I
> developed it a while back on an older 0.8 version (sorry it took so long to
> subscribe and send the patch in).
>
> Cheers,
>
> Rys Sommefeldt
> ---
>
> From 6446cf839d2104cd40848e439bf97cd7fd6ccfee Mon Sep 17 00:00:00 2001
> From: Rys Sommefeldt <rsommefeldt@plus.net>
> Date: Fri, 6 Nov 2009 17:14:56 +0000
> Subject: [PATCH] Close fd when done
>
> ---
> shared.c |    9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/shared.c b/shared.c
> index d7b2d5a..d5e54e6 100644
> --- a/shared.c
> +++ b/shared.c
> @@ -404,14 +404,19 @@ int readfile(const char *path, char **buf, size_t
> *size)
>    struct stat st;
>
>    fd = open(path, O_RDONLY);
> -    if (fd == -1)
> +    if (fd == -1) {
> +        close(fd);
>        return errno;
> -    if (fstat(fd, &st))
> +    }

The above change looks bogus. If fd == -1, you close() it anyway?

> +    if (fstat(fd, &st)) {
> +        close(fd);
>        return errno;
> +    }
>    if (!S_ISREG(st.st_mode))
>        return EISDIR;
>    *buf = xmalloc(st.st_size + 1);
>    *size = read_in_full(fd, *buf, st.st_size);
>    (*buf)[*size] = '\0';
> +    close(fd);
>    return (*size == st.st_size ? 0 : errno);
> }
> --
> 1.6.5.2
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [cgit PATCH] Close file descriptor on error in readfile()

[Rys Sommefeldt]

Steven Noonan wrote:
> The above change looks bogus. If fd == -1, you close() it anyway?
>   
Ah, of course, sorry.  I'll redo the patch.
>> +    if (fstat(fd, &st)) {
>> +        close(fd);
>>        return errno;
>> +    }
>>    if (!S_ISREG(st.st_mode))
>>        return EISDIR;
>>    *buf = xmalloc(st.st_size + 1);
>>    *size = read_in_full(fd, *buf, st.st_size);
>>    (*buf)[*size] = '\0';
>> +    close(fd);
>>    return (*size == st.st_size ? 0 : errno);
>> }
>> --
>> 1.6.5.2
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>     
> --
>   


__________ Information from ESET NOD32 Antivirus, version of virus signature database 4580 (20091106) __________

The message was checked by ESET NOD32 Antivirus.

http://www.eset.com

Re: [cgit PATCH] Close file descriptor on error in readfile()

[Rys Sommefeldt]

All,

Sorry for the earlier HTML email, I'd misconfigured my mail client so 
accept my apologies for that (and thanks Steven).  Here's the reworked 
patch:

 From d928507bf4c8727c3848525f4744d7c8507de5e8 Mon Sep 17 00:00:00 2001
From: Rys Sommefeldt <rys@pixeltards.com>
Date: Sat, 7 Nov 2009 12:15:24 +0000
Subject: [PATCH] Close fd on error in readfile()

---
  shared.c |    5 ++++-
  1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/shared.c b/shared.c
index d7b2d5a..a676fa3 100644
--- a/shared.c
+++ b/shared.c
@@ -406,12 +406,15 @@ int readfile(const char *path, char **buf, size_t 
*size)
     fd = open(path, O_RDONLY);
     if (fd == -1)
         return errno;
-   if (fstat(fd, &st))
+   if (fstat(fd, &st)) {
+       close(fd);
         return errno;
+   }
     if (!S_ISREG(st.st_mode))
         return EISDIR;
     *buf = xmalloc(st.st_size + 1);
     *size = read_in_full(fd, *buf, st.st_size);
     (*buf)[*size] = '\0';
+   close(fd);
     return (*size == st.st_size ? 0 : errno);
  }
-- 
1.6.5.2

Re: [cgit PATCH] Close file descriptor on error in readfile()

[Lars Hjemli]

On Sat, Nov 7, 2009 at 13:23, Rys Sommefeldt <rys@pixeltards.com> wrote:
> Sorry for the earlier HTML email, I'd misconfigured my mail client so accept
> my apologies for that (and thanks Steven).  Here's the reworked patch:

Thanks. I've applied the following to my stable branch:

diff --git a/shared.c b/shared.c
index d7b2d5a..a27ab30 100644
--- a/shared.c
+++ b/shared.c
@@ -406,12 +406,17 @@ int readfile(const char *path, char **buf, size_t *size)
        fd = open(path, O_RDONLY);
        if (fd == -1)
                return errno;
-       if (fstat(fd, &st))
+       if (fstat(fd, &st)) {
+               close(fd);
                return errno;
-       if (!S_ISREG(st.st_mode))
+       }
+       if (!S_ISREG(st.st_mode)) {
+               close(fd);
                return EISDIR;
+       }
        *buf = xmalloc(st.st_size + 1);
        *size = read_in_full(fd, *buf, st.st_size);
        (*buf)[*size] = '\0';
+       close(fd);
        return (*size == st.st_size ? 0 : errno);
 }

--
larsh

Re: [cgit PATCH] Close file descriptor on error in readfile()

[Andreas Schwab]

Lars Hjemli <hjemli@gmail.com> writes:

> diff --git a/shared.c b/shared.c
> index d7b2d5a..a27ab30 100644
> --- a/shared.c
> +++ b/shared.c
> @@ -406,12 +406,17 @@ int readfile(const char *path, char **buf, size_t *size)
>         fd = open(path, O_RDONLY);
>         if (fd == -1)
>                 return errno;
> -       if (fstat(fd, &st))
> +       if (fstat(fd, &st)) {
> +               close(fd);
>                 return errno;

The close call can clobber errno.

> -       if (!S_ISREG(st.st_mode))
> +       }
> +       if (!S_ISREG(st.st_mode)) {
> +               close(fd);
>                 return EISDIR;
> +       }
>         *buf = xmalloc(st.st_size + 1);
>         *size = read_in_full(fd, *buf, st.st_size);
>         (*buf)[*size] = '\0';
> +       close(fd);
>         return (*size == st.st_size ? 0 : errno);

Likewise.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [cgit PATCH] Close file descriptor on error in readfile()

[Lars Hjemli]

On Sat, Nov 7, 2009 at 17:14, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
>> diff --git a/shared.c b/shared.c
>> index d7b2d5a..a27ab30 100644
>> --- a/shared.c
>> +++ b/shared.c
>> @@ -406,12 +406,17 @@ int readfile(const char *path, char **buf, size_t *size)
>>         fd = open(path, O_RDONLY);
>>         if (fd == -1)
>>                 return errno;
>> -       if (fstat(fd, &st))
>> +       if (fstat(fd, &st)) {
>> +               close(fd);
>>                 return errno;
>
> The close call can clobber errno.
>
>> -       if (!S_ISREG(st.st_mode))
>> +       }
>> +       if (!S_ISREG(st.st_mode)) {
>> +               close(fd);
>>                 return EISDIR;
>> +       }
>>         *buf = xmalloc(st.st_size + 1);
>>         *size = read_in_full(fd, *buf, st.st_size);
>>         (*buf)[*size] = '\0';
>> +       close(fd);
>>         return (*size == st.st_size ? 0 : errno);
>
> Likewise.

Thanks for noticing. I've applied the following patch on top of the bad one:

From 21f67e7d82986135922aece6b4ebf410a98705bc Mon Sep 17 00:00:00 2001
From: Lars Hjemli <hjemli@gmail.com>
Date: Sat, 7 Nov 2009 18:08:30 +0100
Subject: [PATCH] shared.c: return original errno

Noticed-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 shared.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/shared.c b/shared.c
index a27ab30..9362d21 100644
--- a/shared.c
+++ b/shared.c
@@ -400,15 +400,16 @@ int cgit_close_filter(struct cgit_filter *filter)
  */
 int readfile(const char *path, char **buf, size_t *size)
 {
-       int fd;
+       int fd, e;
        struct stat st;

        fd = open(path, O_RDONLY);
        if (fd == -1)
                return errno;
        if (fstat(fd, &st)) {
+               e = errno;
                close(fd);
-               return errno;
+               return e;
        }
        if (!S_ISREG(st.st_mode)) {
                close(fd);
@@ -416,7 +417,8 @@ int readfile(const char *path, char **buf, size_t *size)
        }
        *buf = xmalloc(st.st_size + 1);
        *size = read_in_full(fd, *buf, st.st_size);
+       e = errno;
        (*buf)[*size] = '\0';
        close(fd);
-       return (*size == st.st_size ? 0 : errno);
+       return (*size == st.st_size ? 0 : e);
 }

-- 
larsh