Re: svn commit: r324007 - head/usr.sbin/mountd

[Emmanuel Vadot]

On Tue, 26 Sep 2017 22:09:53 +1000 (EST)
Bruce Evans  wrote:

> On Tue, 26 Sep 2017, Emmanuel Vadot wrote:
> 
> > On Tue, 26 Sep 2017 13:24:57 +0300
> > Konstantin Belousov  wrote:
> >
> >> On Tue, Sep 26, 2017 at 09:18:18AM +, 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).

 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  
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r324007 - head/usr.sbin/mountd

[Bruce Evans]

On Tue, 26 Sep 2017, Emmanuel Vadot wrote:


On Tue, 26 Sep 2017 13:24:57 +0300
Konstantin Belousov  wrote:


On Tue, Sep 26, 2017 at 09:18:18AM +, 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.


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).

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.

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
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r324007 - head/usr.sbin/mountd

[Emmanuel Vadot]

On Tue, 26 Sep 2017 13:24:57 +0300
Konstantin Belousov  wrote:

> On Tue, Sep 26, 2017 at 09:18:18AM +, 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.

 Thanks,

-- 
Emmanuel Vadot  
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r324007 - head/usr.sbin/mountd

[Konstantin Belousov]

On Tue, Sep 26, 2017 at 09:18:18AM +, 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.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"