[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] libbb: Close up a potential DIR leak
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2010-12-29 0:05:10
Message-ID: 201012290105.10461.vda.linux () googlemail ! com
[Download RAW message or body]

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
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


[prev in list] [next in list] [prev in thread] [next in thread]