On Sun, Dec 29, 2013 at 11:49:29AM -0800, Loganaden Velvindron wrote:
> On Sun, Dec 29, 2013 at 09:51:28AM -0800, patrick keshishian wrote:
> > Hi,
> >
> > Accidentally deleted this message from my inbox. This is
> > a "reconstruction" from mailing list archive.
> >
> > Suggestion/comment below.
> >
> > Earlier today:
> > > Hi All,
> > >
> > > From NetBSD:
> > > Fix file descriptor leak. Found by cppcheck.
> > >
> > > Index: src/libexec/ld.so/ldconfig/ldconfig.c
> > > ===================================================================
> > > RCS file: /cvs/src/libexec/ld.so/ldconfig/ldconfig.c,v
> > > retrieving revision 1.31
> > > diff -u -p -r1.31 ldconfig.c
> > > --- src/libexec/ld.so/ldconfig/ldconfig.c 13 Nov 2013 05:41:42 -0000
> > > 1.31
> > > +++ src/libexec/ld.so/ldconfig/ldconfig.c 29 Dec 2013 11:53:38 -0000
> > > @@ -416,16 +416,16 @@ buildhints(void)
> > > if (write(fd, &hdr, sizeof(struct hints_header)) !=
> > > sizeof(struct hints_header)) {
> > > warn("%s", _PATH_LD_HINTS);
> > > - goto out;
> > > + goto fdout;
> > > }
> > > if (write(fd, blist, hdr.hh_nbucket * sizeof(struct hints_bucket)) !=
> > > hdr.hh_nbucket * sizeof(struct hints_bucket)) {
> > > warn("%s", _PATH_LD_HINTS);
> > > - goto out;
> > > + goto fdout;
> > > }
> > > if (write(fd, strtab, strtab_sz) != strtab_sz) {
> > > warn("%s", _PATH_LD_HINTS);
> > > - goto out;
> > > + goto fdout;
> > > }
> > > if (close(fd) != 0) {
> > > warn("%s", _PATH_LD_HINTS);
> > > @@ -438,6 +438,8 @@ buildhints(void)
> > > }
> > >
> > > ret = 0;
> > > +fdout:
> > > + (void)close(fd);
> > > out:
> > > free(blist);
> > > free(strtab);
> > >
> >
> > Why not a simpler diff where the the close() call is moved
> > after 'out:' label instead of introducing a new label and
> > an additional close() call?
>
> That's equally possible.
> I adapted the diff from the NetBSD fix, which uses the additional
> label.
>
> I guess that it will boil down to the ldconfig maintainer to decide
> which way is better :-)
My comment about "additional close() call" is not entirely
explicit. With the original suggested patch, if the code
reaches the 'ret = 0;' assignment, close(fd) gets called again,
and at this point fd is already closed.
Cheers,
--patrick
> > Like such:
> > (disclaimer: not tested, not even compiled.)
> >
> > Index: ldconfig.c
> > ===================================================================
> > RCS file: /cvs/obsd/src/libexec/ld.so/ldconfig/ldconfig.c,v
> > retrieving revision 1.31
> > diff -u -p -u -p -r1.31 ldconfig.c
> > --- ldconfig.c 13 Nov 2013 05:41:42 -0000 1.31
> > +++ ldconfig.c 29 Dec 2013 15:31:58 -0000
> > @@ -322,7 +322,7 @@ hinthash(char *cp, int vmajor, int vmino
> > int
> > buildhints(void)
> > {
> > - int strtab_sz = 0, nhints = 0, fd, i, ret = -1, str_index = 0;
> > + int strtab_sz = 0, nhints = 0, fd = -1, i, ret = -1, str_index = 0;
> > struct hints_bucket *blist;
> > struct hints_header hdr;
> > struct shlib_list *shp;
> > @@ -427,10 +427,6 @@ buildhints(void)
> > warn("%s", _PATH_LD_HINTS);
> > goto out;
> > }
> > - if (close(fd) != 0) {
> > - warn("%s", _PATH_LD_HINTS);
> > - goto out;
> > - }
> >
> > if (rename(tmpfilenam, _PATH_LD_HINTS) != 0) {
> > warn("%s", _PATH_LD_HINTS);
> > @@ -439,6 +435,8 @@ buildhints(void)
> >
> > ret = 0;
> > out:
> > + if (-1 != fd && close(fd) != 0)
> > + warn("%s", _PATH_LD_HINTS);
> > free(blist);
> > free(strtab);
> > return (ret);
>
