[bug] fd leak when zchdir to TOOLONG paths

[bug] fd leak when zchdir to TOOLONG paths

[Stephane Chazelas]

Reproduce it with:

cd /tmp/
repeat 500 { mkdir blahblahblah; cd blahblahblah }

Then lsof -p $$ >&2
(you may have troubles as the fds may interact with internal fd
handling stdout (1 being closed!)).

A simple fix would be:

--- ../cvs/zsh/Src/compat.c	2002-08-05 13:35:59.000000000 +0100
+++ ./Src/compat.c	2004-11-29 11:54:26.000000000 +0000
@@ -387,10 +387,20 @@
     int currdir = -2;
 
     for (;;) {
-	if (!*dir)
-	    return 0;
-	if (!chdir(dir))
+	if (*dir == '\0') {
+#ifdef HAVE_FCHDIR
+	    if (currdir >= 0)
+		close(currdir);
+#endif
 	    return 0;
+	}
+	if (chdir(dir) == 0) {
+#ifdef HAVE_FCHDIR
+	    if (currdir >= 0)
+		close(currdir);
+#endif
+	    return 0; /* chdir successful */
+	}
 	if ((errno != ENAMETOOLONG && errno != ENOMEM) ||
 	    strlen(dir) < PATH_MAX)
 	    break;


-- 
Stéphane

Re: [bug] fd leak when zchdir to TOOLONG paths

[Peter Stephenson]

Stephane Chazelas wrote:
> Reproduce it with:
> 
> cd /tmp/
> repeat 500 { mkdir blahblahblah; cd blahblahblah }
> 
> Then lsof -p $$ >&2
> (you may have troubles as the fds may interact with internal fd
> handling stdout (1 being closed!)).
> 
> A simple fix would be:

Thanks, I've committed this, slightly abbreviated to fit in with zsh's
dense and opaque coding standards :-/.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

OT: Coding (was Re: [bug] fd leak)

[Bart Schaefer]

On Mon, 29 Nov 2004, Peter Stephenson wrote:

> Thanks, I've committed this, slightly abbreviated to fit in with zsh's
> dense and opaque coding standards :-/.

No one should be allowed to refer to C code as "dense and opaque" until 
they've had a read through the procmail sources.

zsh coding standards

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

On Mon, Nov 29, 2004 at 12:10:09PM +0000, Peter Stephenson wrote:
> Thanks, I've committed this, slightly abbreviated to fit in with zsh's
> dense and opaque coding standards :-/.

Speaking of coding standards, it has only been fairly recently (perhaps
this year?) that you have started to put braces on the line following an
"if", "while", etc.  Is this a conscious decision to change zsh's coding
style?  I personally prefer seeing the braces on the same line as the
controlling statement (except for the start of a function).

Some other things I think improve readability:

Use "if (chdir(dir) == 0)" to test for success instead of
"if (!chdir(dir))" since the latter reads "NOT chdir", which reads like
we're testing for failure.

Similarly, use "if (chdir(dir) < 0)" to test for failure instead of
"if (chdir(dir))" since the latter reads like we're testing for success.

Use "{}" on an empty loop instead of ";" to make it more obvious that no
looping statements follow.

Debate?

Attached is a patch of the zchdir() function that contains only the style
changes mentioned above.  They won't be committed unless agreement is
reached that they would be a good thing.

Note also that I just checked in a fix for zchdir()'s function comment
(it said it returns 0 on normal error when it really returns -1).  I
made a simple optimization of the error-return code at the same time.

..wayne..

[-- Attachment #2: zchdir.patch --]
[-- Type: text/plain, Size: 964 bytes --]

--- Src/compat.c	29 Nov 2004 16:26:12 -0000	1.14
+++ Src/compat.c	29 Nov 2004 16:28:03 -0000
@@ -387,8 +387,7 @@ zchdir(char *dir)
     int currdir = -2;
 
     for (;;) {
-	if (!*dir || !chdir(dir))
-	{
+	if (!*dir || chdir(dir) == 0) {
 #ifdef HAVE_FCHDIR
            if (currdir >= 0)
                close(currdir);
@@ -398,7 +397,7 @@ zchdir(char *dir)
 	if ((errno != ENAMETOOLONG && errno != ENOMEM) ||
 	    strlen(dir) < PATH_MAX)
 	    break;
-	for (s = dir + PATH_MAX - 1; s > dir && *s != '/'; s--);
+	for (s = dir + PATH_MAX - 1; s > dir && *s != '/'; s--) {}
 	if (s == dir)
 	    break;
 #ifdef HAVE_FCHDIR
@@ -406,7 +405,7 @@ zchdir(char *dir)
 	    currdir = open(".", O_RDONLY|O_NOCTTY);
 #endif
 	*s = '\0';
-	if (chdir(dir)) {
+	if (chdir(dir) < 0) {
 	    *s = '/';
 	    break;
 	}
@@ -414,7 +413,7 @@ zchdir(char *dir)
 	currdir = -1;
 #endif
 	*s = '/';
-	while (*++s == '/');
+	while (*++s == '/') {}
 	dir = s;
     }
 #ifdef HAVE_FCHDIR

Re: zsh coding standards

[Peter Stephenson]

Wayne Davison wrote:
> Speaking of coding standards, it has only been fairly recently (perhaps
> this year?) that you have started to put braces on the line following an
> "if", "while", etc.  Is this a conscious decision to change zsh's coding
> style?  I personally prefer seeing the braces on the same line as the
> controlling statement (except for the start of a function).

It's because that's how we do it at CSR, and switching between the two
is too complicated for me.  It's not intentional.

> Use "{}" on an empty loop instead of ";" to make it more obvious that no
> looping statements follow.

I think this is mostly done by putting the ";" on the next line, which
makes the intention clear enough.  Putting a ";" on the same line
immediately after the loop statement is certainly more bug-prone.

> Attached is a patch of the zchdir() function that contains only the style
> changes mentioned above.  They won't be committed unless agreement is
> reached that they would be a good thing.

Anything that makes it more readable is fine by me, although it's quite
hard to enforce this sort of nicety.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: zsh coding standards

[Alexandre Duret-Lutz]

On Mon, Nov 29, 2004 at 08:42:53AM -0800, Wayne Davison wrote:
>
> Use "{}" on an empty loop instead of ";" to make it more obvious that no
> looping statements follow.

     while (*++s == '/')
       continue;

I discovered this very readable style in Bison's sources recently, and
adopted it.