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"

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

[Emmanuel Vadot]

Author: manu
Date: Tue Sep 26 09:18:18 2017
New Revision: 324007
URL: https://svnweb.freebsd.org/changeset/base/324007

Log:
  mountd: Replace malloc+strcpy to strdup
  
  Reviewed by:  bapt
  MFC after:1 week
  Sponsored by: Gandi.net
  Differential Revision:https://reviews.freebsd.org/D12503

Modified:
  head/usr.sbin/mountd/mountd.c

Modified: head/usr.sbin/mountd/mountd.c
==
--- head/usr.sbin/mountd/mountd.c   Tue Sep 26 09:01:56 2017
(r324006)
+++ head/usr.sbin/mountd/mountd.c   Tue Sep 26 09:18:18 2017
(r324007)
@@ -101,7 +101,7 @@ struct dirlist {
struct dirlist  *dp_right;
int dp_flag;
struct hostlist *dp_hosts;  /* List of hosts this dir exported to */
-   chardp_dirp[1]; /* Actually malloc'd to size of dir */
+   char*dp_dirp;
 };
 /* dp_flag bits */
 #defineDP_DEFSET   0x1
@@ -1525,12 +1525,8 @@ get_exportlist_one(void)
if (ep == (struct exportlist *)NULL) {
ep = get_exp();
ep->ex_fs = fsb.f_fsid;
-   ep->ex_fsdir = (char *)malloc
-   (strlen(fsb.f_mntonname) + 1);
-   if (ep->ex_fsdir)
-   strcpy(ep->ex_fsdir,
-   fsb.f_mntonname);
-   else
+   ep->ex_fsdir = 
strdup(fsb.f_mntonname);
+   if (ep->ex_fsdir == NULL)
out_of_mem();
if (debug)
warnx(
@@ -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));
if (dp == (struct dirlist *)NULL)
out_of_mem();
dp->dp_left = *dpp;
dp->dp_right = (struct dirlist *)NULL;
dp->dp_flag = 0;
dp->dp_hosts = (struct hostlist *)NULL;
-   strcpy(dp->dp_dirp, cp);
+   dp->dp_dirp = strndup(cp, len);
+   if (dp->dp_dirp == NULL)
+   out_of_mem();
*dpp = dp;
return (dp->dp_dirp);
 }
___
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"