Close file descriptor on error in readfile()
PATCH 1/1 Close file descriptor on error in readfile()
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 <[email protected]>
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(-)
shared.c +7 -2
@@ -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);
}
Signature · 1 line
1.6.5.2
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 <[email protected]>
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(-)
shared.c +4 -1
@@ -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); }
Signature · 1 line
1.6.5.2
Quoted 2 lines · Rys Sommefeldt
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:
shared.c +7 -2
@@ -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
shared.c int readfile(const char *path, char **buf, size_t *size) · line 411
Quoted 12 lines · Lars Hjemli
shared.c +2
-1@@ -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.
shared.c int readfile(const char *path, char **buf, size_t *size) · line 421
Quoted 11 lines
shared.c +5
-1-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.
Signature · 3 lines
Andreas Schwab, [email protected] GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Quoted 29 lines · Andreas Schwab
shared.c int readfile(const char *path, char **buf, size_t *size) · line 411
Quoted 12 lines · Lars Hjemli
shared.c +2
-1@@ -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.
shared.c int readfile(const char *path, char **buf, size_t *size) · line 421
Quoted 11 lines
shared.c +5
-1- 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);
Quoted 1 line
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 <[email protected]>
Date: Sat, 7 Nov 2009 18:08:30 +0100
Subject: [PATCH] shared.c: return original errno
--- shared.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
shared.c +5 -3
@@ -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); }
Signature · 1 line
larsh
Quoted 31 lines · 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
6446cf839d2104cd40848e439bf97cd7fd6ccfeeMon Sep 17 00:00:00 2001
From: Rys Sommefeldt <[email protected]>
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(-)shared.c +0
-0
shared.c int readfile(const char *path, char **buf, size_t · line 411
Quoted 10 lines
shared.c +3
-2@@ -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?
Quoted 19 lines
shared.c +4
-0+ 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines · Steven Noonan
The above change looks bogus. If fd == -1, you
close()it anyway?
Ah, of course, sorry. I'll redo the patch.
Quoted 20 lines
Quoted 11 lines
shared.c +4
-0+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);Quoted 9 lines
}
--
1.6.5.2
--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines
--
__________ Information from ESET NOD32 Antivirus, version of virus signature database 4580 (20091106) __________
The message was checked by ESET NOD32 Antivirus.