PATCH: Fix leak during Y shortcircuit glob qualifier

PATCH: Fix leak during Y shortcircuit glob qualifier

[Mikael Magnusson]

The closedir(lock) is the leak I actually did see in usage, I have no
idea if the other part is needed/harmful though. I'll commit just the
closedir if nobody has any opinions.

---
 Src/glob.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Src/glob.c b/Src/glob.c
index 4db6570..8fbefc9 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -534,7 +534,7 @@ scanner(Complist q, int shortcircuit)
 		    if (!closure || !statfullpath("", NULL, 1)) {
 			scanner((q->closure) ? q : q->next, shortcircuit);
 			if (shortcircuit && shortcircuit == matchct)
-			    return;
+			    goto freedir;
 		    }
 		    pathbuf[pathpos = oppos] = '\0';
 		}
@@ -544,7 +544,7 @@ scanner(Complist q, int shortcircuit)
 		str = dupstrpfx(str, l);
 	    insert(str, 0);
 	    if (shortcircuit && shortcircuit == matchct)
-		return;
+		goto freedir;
 	}
     } else {
 	/* Do pattern matching on current path section. */
@@ -635,8 +635,10 @@ scanner(Complist q, int shortcircuit)
 		} else {
 		    /* if the last filename component, just add it */
 		    insert(fn, 1);
-		    if (shortcircuit && shortcircuit == matchct)
-			return;
+		    if (shortcircuit && shortcircuit == matchct) {
+			closedir(lock);
+			goto freedir;
+		    }
 		}
 	    }
 	}
@@ -659,6 +661,7 @@ scanner(Complist q, int shortcircuit)
 	    hrealloc(subdirs, subdirlen, 0);
 	}
     }
+freedir:
     if (pbcwdsav < pathbufcwd) {
 	if (restoredir(&ds))
 	    zerr("current directory lost during glob");
-- 
2.5.0

Re: PATCH: Fix leak during Y shortcircuit glob qualifier

[Bart Schaefer]

On Sep 23, 11:54pm, Mikael Magnusson wrote:
} Subject: PATCH: Fix leak during Y shortcircuit glob qualifier
}
} The closedir(lock) is the leak I actually did see in usage, I have no
} idea if the other part is needed/harmful though. I'll commit just the
} closedir if nobody has any opinions.

Since scanner() is being called recursively, it's not clear that the
recursive call won't have already done an equivalent restoredir()
in the first two "return" cases -- but I do suspect it's needed in
the case where the closedir(lock) plugs a leak.

Is the current directory munged in the situation where you found a leak?

Re: PATCH: Fix leak during Y shortcircuit glob qualifier

[Mikael Magnusson]

On Thu, Sep 24, 2015 at 5:12 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Sep 23, 11:54pm, Mikael Magnusson wrote:
> } Subject: PATCH: Fix leak during Y shortcircuit glob qualifier
> }
> } The closedir(lock) is the leak I actually did see in usage, I have no
> } idea if the other part is needed/harmful though. I'll commit just the
> } closedir if nobody has any opinions.
>
> Since scanner() is being called recursively, it's not clear that the
> recursive call won't have already done an equivalent restoredir()
> in the first two "return" cases -- but I do suspect it's needed in
> the case where the closedir(lock) plugs a leak.
>
> Is the current directory munged in the situation where you found a leak?

Oops, I see that I forgot to include the actual test case in the
message. I think I had it typed out in gmail and then decided to look
into the code instead and forgot to put it in the git send-email.

% : ./*(-.Y1N); ls -l /proc/$$/fd

was my test case, so not much would happen to $PWD in that particular
case. I tried */*/*(-.Y5N) instead now, and nothing untoward seemed to
happen then either (without and with my patch, so no weird pwd
changing or errors about double frees, respectively).

-- 
Mikael Magnusson

Re: PATCH: Fix leak during Y shortcircuit glob qualifier

[Bart Schaefer]

On Sep 24,  5:38am, Mikael Magnusson wrote:
}
} % : ./*(-.Y1N); ls -l /proc/$$/fd
} 
} was my test case, so not much would happen to $PWD in that particular
} case. I tried */*/*(-.Y5N) instead now, and nothing untoward seemed to
} happen then either (without and with my patch, so no weird pwd
} changing or errors about double frees, respectively).

Glancing at scanner() in glob.c and lchdir() in utils.c, it appears that
the working directory would only be changed if the full path to the target
directory is VERY long, i.e., does not fit in a PATH_MAX buffer.  So it
probably is necessary to restore, but also probably really difficult to
construct an example actually goes through that code path.

Re: PATCH: Fix leak during Y shortcircuit glob qualifier

[Mikael Magnusson]

On Thu, Sep 24, 2015 at 7:18 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Sep 24,  5:38am, Mikael Magnusson wrote:
> }
> } % : ./*(-.Y1N); ls -l /proc/$$/fd
> }
> } was my test case, so not much would happen to $PWD in that particular
> } case. I tried */*/*(-.Y5N) instead now, and nothing untoward seemed to
> } happen then either (without and with my patch, so no weird pwd
> } changing or errors about double frees, respectively).
>
> Glancing at scanner() in glob.c and lchdir() in utils.c, it appears that
> the working directory would only be changed if the full path to the target
> directory is VERY long, i.e., does not fit in a PATH_MAX buffer.  So it
> probably is necessary to restore, but also probably really difficult to
> construct an example actually goes through that code path.

I've gone the safe route for now and only committed the closedir(lock) part.

-- 
Mikael Magnusson