git

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

7 messages

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

Reply Threaded

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 <[hidden email]>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Steven Noonan

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

Reply Threaded

On Fri, Nov 6, 2009 at 6:01 PM, Rys Sommefeldt <[hidden email]> 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 <[hidden email]>
> 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 [hidden email]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rys Sommefeldt

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

Reply Threaded

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 [hidden email]
>> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rys Sommefeldt

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

Reply Threaded

In reply to this post by 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 <[hidden email]>
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lars Hjemli

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

Reply Threaded

On Sat, Nov 7, 2009 at 13:23, Rys Sommefeldt <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreas Schwab

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

Reply Threaded

Lars Hjemli <[hidden email]> 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, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lars Hjemli

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

Reply Threaded

On Sat, Nov 7, 2009 at 17:14, Andreas Schwab <[hidden email]> wrote:

> Lars Hjemli <[hidden email]> 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 <[hidden email]>
Date: Sat, 7 Nov 2009 18:08:30 +0100
Subject: [PATCH] shared.c: return original errno

Noticed-by: Andreas Schwab <[hidden email]>
Signed-off-by: Lars Hjemli <[hidden email]>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at http://vger.kernel.org/majordomo-info.html