threads / patch / 21510

patchClose file descriptor on error in readfile()

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

## tl;dr

7 messages between Nov 7, 2009 and Nov 7, 2009. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Rys Sommefeldt· Nov 7, 2009, 02:01 UTC · lore
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(-)
Show changes to shared.c +7 −2
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
Steven Noonan· Nov 7, 2009, 02:22 UTC · re: Rys Sommefeldt · lore

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

On Fri, Nov 6, 2009 at 6:01 PM, Rys Sommefeldt <rys@pixeltards.com> wrote:
Show 41 quoted lines
> 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?
Show 19 quoted lines
> +    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
>
Rys Sommefeldt· Nov 7, 2009, 02:26 UTC · re: Steven Noonan · lore

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

Steven Noonan wrote:
> The above change looks bogus. If fd == -1, you close() it anyway?
>   
Ah, of course, sorry.  I'll redo the patch.
Show 22 quoted lines
>> +    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
Rys Sommefeldt· Nov 7, 2009, 12:23 UTC · re: Rys Sommefeldt · lore

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

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(-)
Show changes to shared.c +4 −1
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
Lars Hjemli· Nov 7, 2009, 14:59 UTC · re: Rys Sommefeldt · lore

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

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:
Show changes to shared.c +7 −3
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
Andreas Schwab· Nov 7, 2009, 16:14 UTC · re: Lars Hjemli · lore

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

Lars Hjemli <hjemli@gmail.com> writes:
Show 12 quoted lines
> 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.
Show 11 quoted lines
> -       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."
Lars Hjemli· Nov 7, 2009, 17:15 UTC · re: Andreas Schwab · lore

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

On Sat, Nov 7, 2009 at 17:14, Andreas Schwab <schwab@linux-m68k.org> wrote:
Show 30 quoted lines
> 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(-)
Show changes to shared.c +5 −3
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

← back to recent threads