Re: [PATCH] Make git-clean a builtin
- From
Shawn Bohrer <shawn.bohrer@gmail.com>
- Date
- Oct 7, 2007, 01:13 UTC
- Message-ID
- <20071007011331.GC5642@mediacenter.austin.rr.com>
- In-Reply-To
- <20071006215253.GX31659@planck.djpig.de>
On Sat, Oct 06, 2007 at 11:52:53PM +0200, Frank Lichtenheld wrote:
Show 25 quoted lines
> On Sat, Oct 06, 2007 at 03:54:06PM -0500, Shawn Bohrer wrote:
> > +static int remove_directory(const char *path)
> > +{
> > + DIR *d;
> > + struct dirent *dir;
> > + d = opendir(path);
> > + if (d) {
> > + chdir(path);
> > + while ((dir = readdir(d)) != NULL) {
> > + if(strcmp( dir->d_name, ".") == 0 ||
> > + strcmp( dir->d_name, ".." ) == 0 )
> > + continue;
> > + if (dir->d_type == DT_DIR)
> > + remove_directory(dir->d_name);
> > + else
> > + unlink(dir->d_name);
> > + }
> > + }
> > + closedir(d);
> > + chdir("..");
> > + return rmdir(path);
> > +}
>
> The unconditional chdir(..) after the conditional chdir(path) seems like
> asking for trouble to me...Agreed, I'm not sure what I was thinking there.
Show 9 quoted lines
> > + while (fgets(path, sizeof(path), cmd_fout) != NULL) {
> > + struct stat st;
> > + char *p;
> > + p = strrchr(path, '\n');
> > + if ( p != NULL )
> > + *p = '\0';
>
> What happens in case p == NULL? It simply tries to remove the partial
> path?If p == NULL then the path didn't have a EOL character. This shouldn't ever really happen since fgets() leaves the EOL character as part of the string, and it is processing the output of git-ls-files which will provide one path per line. If it does happen for some reason then either we will happily remove the file/directory, or if the path is garbage then we will simply fail to remove anything.
Show 8 quoted lines
> > + fclose(cmd_fout);
> > + finish_command(&cmd);
> > + if (!ignored && !access(git_path("info/exclude"), F_OK))
> > + free(buf);
>
> There is a race condition here of the value of access() changes between
> the two calls. Not one likely to trigger but it should be easy to avoid
> alltogether.Yes, though unlikely I agree this wasn't very smart in the first place so I fixed it and will send reworked patch soon.
-- Shawn