On Tuesday 28 December 2010 10:37, Lauri Kasanen wrote:
> > On Monday 27 December 2010 10:02, Lauri Kasanen wrote:
> > > Hi
> > >
> > > find_block_device_in_dir has a potential DIR leak.
> >
> > len = strlen(ap->devpath);
> > rem = DEVNAME_MAX-2 - len;
> > - if (rem <= 0)
> > + if (rem <= 0) {
> > + closedir(dir);
> > return NULL;
> > + }
> >
> > It can be achieved simply by moving this code block up
> > before opendir.
> >
> > Fixed in git.
>
> That's true, but it would cause worse performance IMHO. Isn't the check for
> being a proper dir more lightweight than the strlen one?
> Now there's a strlen done for every file, and then there's the dir check;
> before it was strlen only for the directories.
opendir() is way more expensive than strlen(), so it'll dwarf strlen overhead
in most cases anyway, ... but see this:
while ((entry = readdir(dir)) != NULL) {
safe_strncpy(ap->devpath + len, entry->d_name, rem);
/* lstat: do not follow links */
if (lstat(ap->devpath, &ap->st) != 0)
continue;
if (S_ISBLK(ap->st.st_mode) && ap->st.st_rdev == ap->dev) {
retpath = xstrdup(ap->devpath);
break;
}
if (S_ISDIR(ap->st.st_mode)) { <======================
/* Do not recurse for '.' and '..' */
if (DOT_OR_DOTDOT(entry->d_name))
continue;
retpath = find_block_device_in_dir(ap); <======
if (retpath)
break;
}
we call find_block_device_in_dir() resursively *on directories only*,
so opendir inside wasn't (usually, sans fs errors) returning NULL,
and we were falling into strlen anyway => swapping them doesn't
increase number of strlen calls.
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
