Date: Tue, 26 Sep 2017 14:16:30 +0200 From: Emmanuel Vadot <manu@bidouilliste.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Konstantin Belousov <kostikbel@gmail.com>, Emmanuel Vadot <manu@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r324007 - head/usr.sbin/mountd Message-ID: <20170926141630.9bdf32c09ac3a71e33e08b68@bidouilliste.com> In-Reply-To: <20170926212215.T1299@besplex.bde.org> References: <201709260918.v8Q9II14056929@repo.freebsd.org> <20170926102457.GG2271@kib.kiev.ua> <20170926131135.73b1f216e9448d863f7b48a9@bidouilliste.com> <20170926212215.T1299@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 26 Sep 2017 22:09:53 +1000 (EST)
Bruce Evans <brde@optusnet.com.au> wrote:
> On Tue, 26 Sep 2017, Emmanuel Vadot wrote:
>
> > On Tue, 26 Sep 2017 13:24:57 +0300
> > Konstantin Belousov <kostikbel@gmail.com> wrote:
> >
> >> On Tue, Sep 26, 2017 at 09:18:18AM +0000, Emmanuel Vadot wrote:
> >>> @@ -1940,14 +1936,16 @@ add_expdir(struct dirlist **dpp, char *cp, int len)
> >>> {
> >>> struct dirlist *dp;
> >>>
> >>> - dp = (struct dirlist *)malloc(sizeof (struct dirlist) + len);
> >>> + dp = (struct dirlist *)malloc(sizeof (struct dirlist));
> >> You might remove the unneeded cast as well.
> >
> > Right, done in 324012.
Hi Bruce,
> Er, mountd has many similar casts, not just the one fixed, and worse ones
> like casting dp to caddr_t before passing it to free() (caddr_t is bogus,
> and free() doesn't actually take it -- its prototype converts caddr_t to
> void *, just like it would convert dp's type to void *)). Some of these
> casts were needed in 1987 for unprototyped pre-StandardC code. caddr_t
> was more needed in 1977 before void * existed (free() takes a char * arg
> in K&R1).
I've fixed this one in 324014 and will probably apply style(9) to the
whole file at some point.
> But the main hug near here is leaking memory for strdup()). The above
> malloc() allocates 2 storage areas together (1 for the string at the
> end), and seems to have a corresponding free(). Now there is an extra
> separate storage error for the string and no separate free() for it.
Yes, sorry for that, this is fixed in 324014 too.
> This is not much more than a style bug too. mountd is a long-lived
> daemon, so it should avoid leaking memory, but it would probably work
> OK for a few thousand mounts with no free()s at all or a few million
> with no frees of strings. But it tries to free() most things, so it
> is a style bug to not try for some.
>
> Bruce
Thanks,
--
Emmanuel Vadot <manu@bidouilliste.com> <manu@freebsd.org>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170926141630.9bdf32c09ac3a71e33e08b68>