Possible bugs in nfs-utils

Possible bugs in nfs-utils

[Michael Halcrow]

[-- Attachment #1.1: Type: text/plain, Size: 2600 bytes --]

I ran nfs-utils through the Coverity source code scanner, and the
following items were flagged that I thought might merit a second
glance (source from util-linux-2.13-pre7.tar.bz2). Those with a more
intimate knowledge of the code base can probably help out in
determining whether some of these really are problematic:

---
support/nfs/svc_socket.c::svc_socket():

If ret == 0 and rpcp == NULL, then servp is checked for non-NULL
status before it is initialized.
---

---
utils/rquotad/rquota_server.c::getquotainfo():

qfpathname may be leaked.
---

---
utils/statd/notlist.c::nlist_new():

new may be leaked here:
        if (!(NL_MY_NAME(new) = xstrdup(my_name))
                        || !(NL_MON_NAME(new) = xstrdup(mon_name)))
                return NULL;
---

---
tools/rpcgen/rpc_parse.c::get_definition():

defp may be leaked on tok.kind == TOK_EOF.
---

---
utils/idmapd/cfg.c::conf_get_tag_list():

node may be leaked here:
        if (!node->field)
          goto cleanup;
...
 cleanup:
  if (list)
    conf_free_list (list);
  return 0;

Same issue in conf_get_list().
---

---
support/misc/mountpoint.c::is_mountpoint():

No check for NULL result from malloc here:
        dotdot = malloc(strlen(path)+4);
        strcat(strcpy(dotdot, path), "/..");

dotdot not freed prior to return.
---

---
utils/idmapd/cfg.c::conf_remove():

Dereference NULL pointer:
  node = conf_trans_node (transaction, CONF_REMOVE);
  if (!node)
    goto fail;
...
 fail:
  if (node->section)
    free (node->section);
---

---
tools/rpcgen/rpc_parse.c::def_const():

Dead code here:
    flag=0;
    if(peekscan(TOK_CASE,&tok))
      {

        do
          {
            scan2(TOK_IDENT, TOK_CHARCONST, &tok);
            cases->contflag=1;  /* continued case statement */
            *tailp = cases;
            tailp = &cases->next;
            cases = ALLOC(case_list);
            cases->case_name = tok.str;
            scan(TOK_COLON, &tok);

          }while(peekscan(TOK_CASE,&tok));
      }
    else
      if(flag)
        {

          *tailp = cases;
          tailp = &cases->next;
          cases = ALLOC(case_list);
        };

It looks like flag will always be 0 at the if(flag) check.
---

---
utils/idmapd/idmapd.c::mydaemon():

tempfd not checked (not likely to be a problem for /dev/null, but just
in case):
                tempfd = open("/dev/null", O_RDWR);
                dup2(tempfd, 0);
---

---
support/nfs/cacheio.c::cache_flush():

Return value not checked:
stat(_PATH_ETAB, &stb);
---

---
tools/rpcgen/rpc_scan.c::docppline():

Return without freeing storage.
---

Thanks,
Mike

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 481 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Possible bugs in nfs-utils

[Michael Halcrow]

[-- Attachment #1.1: Type: text/plain, Size: 195 bytes --]

On Mon, Jun 19, 2006 at 03:30:25PM -0500, Michael Halcrow wrote:
> (source from util-linux-2.13-pre7.tar.bz2).

Oops; this is nonsense. It's nfs-cvs-20060606.tar.gz. Sorry about
the mixup.

Mike

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 481 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Possible bugs in nfs-utils

[Greg Banks]

On Tue, 2006-06-20 at 06:30, Michael Halcrow wrote:
> I ran nfs-utils through the Coverity source code scanner, and the
> following items were flagged that I thought might merit a second
> glance (source from util-linux-2.13-pre7.tar.bz2). Those with a more
> intimate knowledge of the code base can probably help out in
> determining whether some of these really are problematic:

Some of these look worth fixing.  Neil, I have a bunch of
trivial nfs-utils compile warnings fixes waiting to be
pushed, shall I add these in too?

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.



All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=107521&bid=248729&dat=121642
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Possible bugs in nfs-utils

[Neil Brown]

On Thursday June 22, gnb@melbourne.sgi.com wrote:
> On Tue, 2006-06-20 at 06:30, Michael Halcrow wrote:
> > I ran nfs-utils through the Coverity source code scanner, and the
> > following items were flagged that I thought might merit a second
> > glance (source from util-linux-2.13-pre7.tar.bz2). Those with a more
> > intimate knowledge of the code base can probably help out in
> > determining whether some of these really are problematic:
> 
> Some of these look worth fixing.  Neil, I have a bunch of
> trivial nfs-utils compile warnings fixes waiting to be
> pushed, shall I add these in too?

Yeh, send 'em on down.
Or if you happen to have them in a .git somewhere I could pull from
that.  I haven't done a real 'git pull' yet and would like an excuse
to experiment with it.

But patches in Email are fine too.

Thanks,
NeilBrown

All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=107521&bid=248729&dat=121642
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Possible bugs in nfs-utils

[Greg Banks]

On Thu, 2006-06-22 at 18:52, Neil Brown wrote:
> On Thursday June 22, gnb@melbourne.sgi.com wrote:
> > On Tue, 2006-06-20 at 06:30, Michael Halcrow wrote:
> > > I ran nfs-utils through the Coverity source code scanner, and the
> > > following items were flagged that I thought might merit a second
> > > glance (source from util-linux-2.13-pre7.tar.bz2). Those with a more
> > > intimate knowledge of the code base can probably help out in
> > > determining whether some of these really are problematic:
> > 
> > Some of these look worth fixing.  Neil, I have a bunch of
> > trivial nfs-utils compile warnings fixes waiting to be
> > pushed, shall I add these in too?
> 
> Yeh, send 'em on down.
> Or if you happen to have them in a .git somewhere

I do, but not publicly available yet.  I can probably
have something up on oss.sgi.com in a few days.

> I could pull from
> that.  I haven't done a real 'git pull' yet and would like an excuse
> to experiment with it.

Sounds like fun.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.



Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Possible bugs in nfs-utils

[Neil Brown]

On Monday June 19, mhalcrow@us.ibm.com wrote:
> I ran nfs-utils through the Coverity source code scanner, and the
> following items were flagged that I thought might merit a second
> glance (source from util-linux-2.13-pre7.tar.bz2). Those with a more
> intimate knowledge of the code base can probably help out in
> determining whether some of these really are problematic:
> 
> ---
> support/nfs/svc_socket.c::svc_socket():
> 
> If ret == 0 and rpcp == NULL, then servp is checked for non-NULL
> status before it is initialized.
> ---

I think that combination should be impossible, but I cannot find a
manpage for getrpcbynumber_r to conform it, so I'll just initialise
servp to NULL - it doesn't hurt.

> 
> ---
> utils/rquotad/rquota_server.c::getquotainfo():
> 
> qfpathname may be leaked.
> ---

Oh, the indenting in that file is HORRIBLE...

Yes, I'll move the free(qfpathname) out to a more appropriate level.

> 
> ---
> utils/statd/notlist.c::nlist_new():
> 
> new may be leaked here:
>         if (!(NL_MY_NAME(new) = xstrdup(my_name))
>                         || !(NL_MON_NAME(new) = xstrdup(mon_name)))
>                 return NULL;
> ---

If it is, we are short of memory anyway and will probably die soon,
but I've added a couple of appropriate frees.

> 
> ---
> tools/rpcgen/rpc_parse.c::get_definition():
> 
> defp may be leaked on tok.kind == TOK_EOF.
> ---

Yep.


> 
> ---
> utils/idmapd/cfg.c::conf_get_tag_list():
> 
> node may be leaked here:
>         if (!node->field)
>           goto cleanup;
> ...
>  cleanup:
>   if (list)
>     conf_free_list (list);
>   return 0;
> 
> Same issue in conf_get_list().
> ---

Ok.

> 
> ---
> support/misc/mountpoint.c::is_mountpoint():
> 
> No check for NULL result from malloc here:
>         dotdot = malloc(strlen(path)+4);
>         strcat(strcpy(dotdot, path), "/..");
> 
> dotdot not freed prior to return.
> ---

Fixed.

> 
> ---
> utils/idmapd/cfg.c::conf_remove():
> 
> Dereference NULL pointer:
>   node = conf_trans_node (transaction, CONF_REMOVE);
>   if (!node)
>     goto fail;
> ...
>  fail:
>   if (node->section)
>     free (node->section);
> ---
Make it
	if (node && node->selection)

> 
> ---
> tools/rpcgen/rpc_parse.c::def_const():

You mean def_union.

> 
> Dead code here:
>     flag=0;
>     if(peekscan(TOK_CASE,&tok))
>       {
> 
>         do
>           {
>             scan2(TOK_IDENT, TOK_CHARCONST, &tok);
>             cases->contflag=1;  /* continued case statement */
>             *tailp = cases;
>             tailp = &cases->next;
>             cases = ALLOC(case_list);
>             cases->case_name = tok.str;
>             scan(TOK_COLON, &tok);
> 
>           }while(peekscan(TOK_CASE,&tok));
>       }
>     else
>       if(flag)
>         {
> 
>           *tailp = cases;
>           tailp = &cases->next;
>           cases = ALLOC(case_list);
>         };
> 
> It looks like flag will always be 0 at the if(flag) check.
> ---

Yes.  It appears that once upon a time the code parsed

  case 0:
  case 1:
     stuff

by setting the flag if stuff was (or maybe wasn't) found, so it could
deal with multiple labels for the same stuff.  But it was changed to
peek and the next token and if it was 'case', do the special handling
then.  So 'flag' is no longer needed.  I've removed it.


> 
> ---
> utils/idmapd/idmapd.c::mydaemon():
> 
> tempfd not checked (not likely to be a problem for /dev/null, but just
> in case):
>                 tempfd = open("/dev/null", O_RDWR);
>                 dup2(tempfd, 0);
> ---

As you say, unlikely, and not really harmful anyway, but I've fixed
it.

> 
> ---
> support/nfs/cacheio.c::cache_flush():
> 
> Return value not checked:
> stat(_PATH_ETAB, &stb);
> ---

That is deliberate (I know, because I wrote that bit :-)
I assume that if stat fails it doesn't change the 'struct stat', in
particular, st_mtime stays at what it was set to.
However I'll make the code more readable.

> 
> ---
> tools/rpcgen/rpc_scan.c::docppline():
> 
> Return without freeing storage.
> ---

Only if *file == 0... fixed.

Below is the patch.  It compiles.
If you (or anyone) would like to review it...

Thanks.
NeilBrown


>From 2e075a16da4963f54cd556403ca9e15a68de27fd Mon Sep 17 00:00:00 2001
From: Neil Brown <neilb@suse.de>
Date: Fri, 23 Jun 2006 14:38:33 +1000
Subject: [PATCH] Fix various issues discovered by Coverity

Thanks to Michael Halcrow for finding them.
---
 support/misc/mountpoint.c     |   18 ++++++++++++------
 support/nfs/cacheio.c         |    6 +++---
 support/nfs/svc_socket.c      |    2 +-
 tools/rpcgen/rpc_parse.c      |   11 +----------
 tools/rpcgen/rpc_scan.c       |    1 +
 utils/idmapd/cfg.c            |   10 +++++++---
 utils/idmapd/idmapd.c         |    8 +++++---
 utils/rquotad/rquota_server.c |    3 ++-
 utils/statd/notlist.c         |    6 +++++-
 9 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/support/misc/mountpoint.c b/support/misc/mountpoint.c
index 6d0f34e..2cf1324 100644
--- a/support/misc/mountpoint.c
+++ b/support/misc/mountpoint.c
@@ -20,15 +20,21 @@ is_mountpoint(char *path)
 	 */
 	char *dotdot;
 	struct stat stb, pstb;
+	int rv;
 
 	dotdot = malloc(strlen(path)+4);
+	if (!dotdot)
+		return 0;
 	strcat(strcpy(dotdot, path), "/..");
 	if (lstat(path, &stb) != 0 ||
 	    lstat(dotdot, &pstb) != 0)
-		return 0;
-
-	if (stb.st_dev != pstb.st_dev
-	    || stb.st_ino == pstb.st_ino)
-		return 1;
-	return 0;
+		rv = 0;
+	else
+		if (stb.st_dev != pstb.st_dev ||
+		    stb.st_ino == pstb.st_ino)
+			rv = 1;
+		else
+			rv = 0;
+	free(dotdot);
+	return rv;
 }
diff --git a/support/nfs/cacheio.c b/support/nfs/cacheio.c
index d7ad429..3e868d8 100644
--- a/support/nfs/cacheio.c
+++ b/support/nfs/cacheio.c
@@ -259,9 +259,9 @@ cache_flush(int force)
 		"nfsd.export",
 		NULL
 	};
-	stb.st_mtime = time(0);
-	if (!force)
-		stat(_PATH_ETAB, &stb);
+	if (force ||
+	    stat(_PATH_ETAB, &stb) != 0)
+		stb.st_mtime = time(0);
 	
 	sprintf(stime, "%ld\n", stb.st_mtime);
 	for (c=0; cachelist[c]; c++) {
diff --git a/support/nfs/svc_socket.c b/support/nfs/svc_socket.c
index 888c915..c41a1a3 100644
--- a/support/nfs/svc_socket.c
+++ b/support/nfs/svc_socket.c
@@ -42,7 +42,7 @@ svc_socket (u_long number, int type, int
   socklen_t len = sizeof (struct sockaddr_in);
   char rpcdata [1024], servdata [1024];
   struct rpcent rpcbuf, *rpcp;
-  struct servent servbuf, *servp;
+  struct servent servbuf, *servp = NULL;
   int sock, ret;
   const char *proto = protocol == IPPROTO_TCP ? "tcp" : "udp";
 
diff --git a/tools/rpcgen/rpc_parse.c b/tools/rpcgen/rpc_parse.c
index 577312e..70d1260 100644
--- a/tools/rpcgen/rpc_parse.c
+++ b/tools/rpcgen/rpc_parse.c
@@ -94,6 +94,7 @@ get_definition(void)
 		def_const(defp);
 		break;
 	case TOK_EOF:
+		free(defp);
 		return (NULL);
 	default:
 		error("definition keyword expected");
@@ -290,7 +291,6 @@ def_union(definition *defp)
   declaration dec;
   case_list *cases;
   case_list **tailp;
-  int flag;
 
   defp->def_kind = DEF_UNION;
   scan(TOK_IDENT, &tok);
@@ -309,7 +309,6 @@ def_union(definition *defp)
     cases->case_name = tok.str;
     scan(TOK_COLON, &tok);
     /* now peek at next token */
-    flag=0;
     if(peekscan(TOK_CASE,&tok))
       {
 
@@ -325,14 +324,6 @@ def_union(definition *defp)
       
 	  }while(peekscan(TOK_CASE,&tok));
       }
-    else
-      if(flag)
-	{
-
-	  *tailp = cases;
-	  tailp = &cases->next;
-	  cases = ALLOC(case_list);
-	};
 
     get_declaration(&dec, DEF_UNION);
     cases->case_decl = dec;
diff --git a/tools/rpcgen/rpc_scan.c b/tools/rpcgen/rpc_scan.c
index 62d906d..51eecfe 100644
--- a/tools/rpcgen/rpc_scan.c
+++ b/tools/rpcgen/rpc_scan.c
@@ -468,6 +468,7 @@ docppline(char *line, int *lineno, char 
 	*p = 0;
 	if (*file == 0) {
 		*fname = NULL;
+		free(file);
 	} else {
 		*fname = file;
 	}
diff --git a/utils/idmapd/cfg.c b/utils/idmapd/cfg.c
index b22a7c9..16d392a 100644
--- a/utils/idmapd/cfg.c
+++ b/utils/idmapd/cfg.c
@@ -487,8 +487,10 @@ conf_get_list (char *section, char *tag)
       if (!node)
 	goto cleanup;
       node->field = strdup (field);
-      if (!node->field)
+      if (!node->field) {
+	free(node);
 	goto cleanup;
+      }
       TAILQ_INSERT_TAIL (&list->fields, node, link);
     }
   free (liststr);
@@ -523,8 +525,10 @@ conf_get_tag_list (char *section)
 	if (!node)
 	  goto cleanup;
 	node->field = strdup (cb->tag);
-	if (!node->field)
+	if (!node->field) {
+	  free(node);
 	  goto cleanup;
+	}
 	TAILQ_INSERT_TAIL (&list->fields, node, link);
       }
   return list;
@@ -708,7 +712,7 @@ conf_remove (int transaction, char *sect
   return 0;
 
  fail:
-  if (node->section)
+  if (node && node->section)
     free (node->section);
   if (node)
     free (node);
diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
index 158feaf..1231db4 100644
--- a/utils/idmapd/idmapd.c
+++ b/utils/idmapd/idmapd.c
@@ -1003,9 +1003,11 @@ mydaemon(int nochdir, int noclose)
 
 	if (noclose == 0) {
 		tempfd = open("/dev/null", O_RDWR);
-		dup2(tempfd, 0);
-		dup2(tempfd, 1);
-		dup2(tempfd, 2);
+		if (tempfd >= 0) {
+			dup2(tempfd, 0);
+			dup2(tempfd, 1);
+			dup2(tempfd, 2);
+		}
 		closeall(3);
 	}
 
diff --git a/utils/rquotad/rquota_server.c b/utils/rquotad/rquota_server.c
index 109c94e..e3715bd 100644
--- a/utils/rquotad/rquota_server.c
+++ b/utils/rquotad/rquota_server.c
@@ -201,7 +201,6 @@ getquota_rslt *getquotainfo(int flags, c
 				      free(qfpathname);
 				      continue;
 			      }
-			      free(qfpathname);
 			      lseek(fd, (long) dqoff(id), L_SET);
 			      switch (read(fd, &dq_dqb, sizeof(struct dqblk))) {
 			      case 0:/* EOF */
@@ -215,6 +214,7 @@ getquota_rslt *getquotainfo(int flags, c
 				      break;
 			      default:   /* ERROR */
 				      close(fd);
+				      free(qfpathname);
 				      continue;
 			      }
 			      close(fd);
@@ -228,6 +228,7 @@ getquota_rslt *getquotainfo(int flags, c
 		      dqb.dqb_btime = dq_dqb.dqb_btime;
 		      dqb.dqb_itime = dq_dqb.dqb_itime;
 	      }
+	      free(qfpathname);
          endmntent(fp);
 
          if (err && (flags & ACTIVE)) {
diff --git a/utils/statd/notlist.c b/utils/statd/notlist.c
index 4f52b1d..98aa6e2 100644
--- a/utils/statd/notlist.c
+++ b/utils/statd/notlist.c
@@ -61,8 +61,12 @@ nlist_new(char *my_name, char *mon_name,
 	NL_TIMES(new) = MAX_TRIES;
 	NL_STATE(new) = state;
 	if (!(NL_MY_NAME(new) = xstrdup(my_name))
-		       	|| !(NL_MON_NAME(new) = xstrdup(mon_name)))
+	    || !(NL_MON_NAME(new) = xstrdup(mon_name))) {
+		if (NL_MY_NAME(new))
+			free(NL_MY_NAME(new));
+		free(new);
 		return NULL;
+	}
 
 	return new;
 }
-- 
1.4.0


Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Possible bugs in nfs-utils

[Greg Banks]

On Fri, 2006-06-23 at 14:39, Neil Brown wrote:
> On Monday June 19, mhalcrow@us.ibm.com wrote:
> > I ran nfs-utils through the Coverity source code scanner, and the
> > following items were flagged that I thought might merit a second

> 
> Below is the patch.  It compiles.
> If you (or anyone) would like to review it...
> 
> Thanks.
> NeilBrown
> 
> 
> >From 2e075a16da4963f54cd556403ca9e15a68de27fd Mon Sep 17 00:00:00 2001
> From: Neil Brown <neilb@suse.de>
> Date: Fri, 23 Jun 2006 14:38:33 +1000
> Subject: [PATCH] Fix various issues discovered by Coverity
> 
> Thanks to Michael Halcrow for finding them.
> ---
>  support/misc/mountpoint.c     |   18 ++++++++++++------
>  support/nfs/cacheio.c         |    6 +++---
>  support/nfs/svc_socket.c      |    2 +-
>  tools/rpcgen/rpc_parse.c      |   11 +----------
>  tools/rpcgen/rpc_scan.c       |    1 +
>  utils/idmapd/cfg.c            |   10 +++++++---
>  utils/idmapd/idmapd.c         |    8 +++++---
>  utils/rquotad/rquota_server.c |    3 ++-
>  utils/statd/notlist.c         |    6 +++++-
>  9 files changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/support/misc/mountpoint.c b/support/misc/mountpoint.c
> index 6d0f34e..2cf1324 100644
> --- a/support/misc/mountpoint.c
> +++ b/support/misc/mountpoint.c
> @@ -20,15 +20,21 @@ is_mountpoint(char *path)
>  	 */
>  	char *dotdot;
>  	struct stat stb, pstb;
> +	int rv;
>  
>  	dotdot = malloc(strlen(path)+4);
> +	if (!dotdot)
> +		return 0;

This isn't right; if malloc() fails you can't conclude that
the argument isn't a mountpoint, and you shouldn't tell the
caller that.

How about using the xmalloc() routine in support/nfs/xcommon.c ?
It cannot return NULL and will exit() with a warning should
malloc() fail, which is better than a subtle logic error.

Hmm, actually, looking at xmalloc() it has a problem.  It
does a gettext() and vfprintf() in the failure path, both
of which might fail when we're out of memory.

>  	strcat(strcpy(dotdot, path), "/..");
>  	if (lstat(path, &stb) != 0 ||
>  	    lstat(dotdot, &pstb) != 0)
> -		return 0;
> -
> -	if (stb.st_dev != pstb.st_dev
> -	    || stb.st_ino == pstb.st_ino)
> -		return 1;
> -	return 0;
> +		rv = 0;
> +	else
> +		if (stb.st_dev != pstb.st_dev ||
> +		    stb.st_ino == pstb.st_ino)
> +			rv = 1;
> +		else
> +			rv = 0;
> +	free(dotdot);
> +	return rv;

Yep.

>  }
> diff --git a/support/nfs/cacheio.c b/support/nfs/cacheio.c
> index d7ad429..3e868d8 100644
> --- a/support/nfs/cacheio.c
> +++ b/support/nfs/cacheio.c
> @@ -259,9 +259,9 @@ cache_flush(int force)
>  		"nfsd.export",
>  		NULL
>  	};
> -	stb.st_mtime = time(0);
> -	if (!force)
> -		stat(_PATH_ETAB, &stb);
> +	if (force ||
> +	    stat(_PATH_ETAB, &stb) != 0)
> +		stb.st_mtime = time(0);
>  	
>  	sprintf(stime, "%ld\n", stb.st_mtime);
>  	for (c=0; cachelist[c]; c++) {

Yep.

> diff --git a/support/nfs/svc_socket.c b/support/nfs/svc_socket.c
> index 888c915..c41a1a3 100644
> --- a/support/nfs/svc_socket.c
> +++ b/support/nfs/svc_socket.c
> @@ -42,7 +42,7 @@ svc_socket (u_long number, int type, int
>    socklen_t len = sizeof (struct sockaddr_in);
>    char rpcdata [1024], servdata [1024];
>    struct rpcent rpcbuf, *rpcp;
> -  struct servent servbuf, *servp;
> +  struct servent servbuf, *servp = NULL;
>    int sock, ret;
>    const char *proto = protocol == IPPROTO_TCP ? "tcp" : "udp";
>  

Yep.

> diff --git a/tools/rpcgen/rpc_parse.c b/tools/rpcgen/rpc_parse.c
> index 577312e..70d1260 100644
> --- a/tools/rpcgen/rpc_parse.c
> +++ b/tools/rpcgen/rpc_parse.c
> @@ -94,6 +94,7 @@ get_definition(void)
>  		def_const(defp);
>  		break;
>  	case TOK_EOF:
> +		free(defp);
>  		return (NULL);
>  	default:
>  		error("definition keyword expected");

Yep.

Interesting that Coverity doesn't complain about the return value
of the ALLOC() macro not being checked for NULL, as ALLOC() is just
a trivial wrapper for malloc().

> @@ -290,7 +291,6 @@ def_union(definition *defp)
>    declaration dec;
>    case_list *cases;
>    case_list **tailp;
> -  int flag;
>  
>    defp->def_kind = DEF_UNION;
>    scan(TOK_IDENT, &tok);
> @@ -309,7 +309,6 @@ def_union(definition *defp)
>      cases->case_name = tok.str;
>      scan(TOK_COLON, &tok);
>      /* now peek at next token */
> -    flag=0;
>      if(peekscan(TOK_CASE,&tok))
>        {
>  
> @@ -325,14 +324,6 @@ def_union(definition *defp)
>        
>  	  }while(peekscan(TOK_CASE,&tok));
>        }
> -    else
> -      if(flag)
> -	{
> -
> -	  *tailp = cases;
> -	  tailp = &cases->next;
> -	  cases = ALLOC(case_list);
> -	};
>  
>      get_declaration(&dec, DEF_UNION);
>      cases->case_decl = dec;

Yep.

> diff --git a/tools/rpcgen/rpc_scan.c b/tools/rpcgen/rpc_scan.c
> index 62d906d..51eecfe 100644
> --- a/tools/rpcgen/rpc_scan.c
> +++ b/tools/rpcgen/rpc_scan.c
> @@ -468,6 +468,7 @@ docppline(char *line, int *lineno, char 
>  	*p = 0;
>  	if (*file == 0) {
>  		*fname = NULL;
> +		free(file);
>  	} else {
>  		*fname = file;
>  	}

Yep.

Same comment about the alloc() macro as about ALLOC().

> diff --git a/utils/idmapd/cfg.c b/utils/idmapd/cfg.c
> index b22a7c9..16d392a 100644
> --- a/utils/idmapd/cfg.c
> +++ b/utils/idmapd/cfg.c
> @@ -487,8 +487,10 @@ conf_get_list (char *section, char *tag)
>        if (!node)
>  	goto cleanup;
>        node->field = strdup (field);
> -      if (!node->field)
> +      if (!node->field) {
> +	free(node);
>  	goto cleanup;
> +      }
>        TAILQ_INSERT_TAIL (&list->fields, node, link);
>      }
>    free (liststr);

Yep (albeit, as you point out, futile).

> @@ -523,8 +525,10 @@ conf_get_tag_list (char *section)
>  	if (!node)
>  	  goto cleanup;
>  	node->field = strdup (cb->tag);
> -	if (!node->field)
> +	if (!node->field) {
> +	  free(node);
>  	  goto cleanup;
> +	}
>  	TAILQ_INSERT_TAIL (&list->fields, node, link);
>        }
>    return list;

Ditto.

> @@ -708,7 +712,7 @@ conf_remove (int transaction, char *sect
>    return 0;
>  
>   fail:
> -  if (node->section)
> +  if (node && node->section)
>      free (node->section);
>    if (node)
>      free (node);

Yep.

> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> index 158feaf..1231db4 100644
> --- a/utils/idmapd/idmapd.c
> +++ b/utils/idmapd/idmapd.c
> @@ -1003,9 +1003,11 @@ mydaemon(int nochdir, int noclose)
>  
>  	if (noclose == 0) {
>  		tempfd = open("/dev/null", O_RDWR);
> -		dup2(tempfd, 0);
> -		dup2(tempfd, 1);
> -		dup2(tempfd, 2);
> +		if (tempfd >= 0) {
> +			dup2(tempfd, 0);
> +			dup2(tempfd, 1);
> +			dup2(tempfd, 2);
> +		}
>  		closeall(3);
>  	}

I don't think this is right.  In the unlikely event that /dev/null
isn't there, we don't want to continue with the original std{in,out,err}
as it might have implications for the process' controlling terminal.
I think it might be better to either exit or try to open something
even more reliable like "/" (being a directory, write()s to it
should fail).


> diff --git a/utils/rquotad/rquota_server.c b/utils/rquotad/rquota_server.c
> index 109c94e..e3715bd 100644
> --- a/utils/rquotad/rquota_server.c
> +++ b/utils/rquotad/rquota_server.c
> @@ -201,7 +201,6 @@ getquota_rslt *getquotainfo(int flags, c
>  				      free(qfpathname);
>  				      continue;
>  			      }
> -			      free(qfpathname);
>  			      lseek(fd, (long) dqoff(id), L_SET);
>  			      switch (read(fd, &dq_dqb, sizeof(struct dqblk))) {
>  			      case 0:/* EOF */
> @@ -215,6 +214,7 @@ getquota_rslt *getquotainfo(int flags, c
>  				      break;
>  			      default:   /* ERROR */
>  				      close(fd);
> +				      free(qfpathname);
>  				      continue;
>  			      }
>  			      close(fd);
> @@ -228,6 +228,7 @@ getquota_rslt *getquotainfo(int flags, c
>  		      dqb.dqb_btime = dq_dqb.dqb_btime;
>  		      dqb.dqb_itime = dq_dqb.dqb_itime;
>  	      }
> +	      free(qfpathname);
>           endmntent(fp);
>  
>           if (err && (flags & ACTIVE)) {

Yep.

> diff --git a/utils/statd/notlist.c b/utils/statd/notlist.c
> index 4f52b1d..98aa6e2 100644
> --- a/utils/statd/notlist.c
> +++ b/utils/statd/notlist.c
> @@ -61,8 +61,12 @@ nlist_new(char *my_name, char *mon_name,
>  	NL_TIMES(new) = MAX_TRIES;
>  	NL_STATE(new) = state;
>  	if (!(NL_MY_NAME(new) = xstrdup(my_name))
> -		       	|| !(NL_MON_NAME(new) = xstrdup(mon_name)))
> +	    || !(NL_MON_NAME(new) = xstrdup(mon_name))) {
> +		if (NL_MY_NAME(new))
> +			free(NL_MY_NAME(new));
> +		free(new);
>  		return NULL;
> +	}
>  
>  	return new;
>  }

Hmm,  it looks like statd has it's own redundant xstrdup()
implementation, but it has the semantics as the other one,
i.e. it never returns NULL.  So these checks for NULL
are superfluous and the code could be simplified thus:


--- a/utils/statd/notlist.c
+++ b/utils/statd/notlist.c
@@ -54,15 +54,13 @@ nlist_new(char *my_name, char *mon_name,
 {
        notify_list     *new;
  
-       if (!(new = (notify_list *) xmalloc(sizeof(notify_list))))
-               return NULL;
+       new = (notify_list *) xmalloc(sizeof(notify_list));
        memset(new, 0, sizeof(*new));
  
        NL_TIMES(new) = MAX_TRIES;
        NL_STATE(new) = state;
-       if (!(NL_MY_NAME(new) = xstrdup(my_name))
-                       || !(NL_MON_NAME(new) = xstrdup(mon_name)))
-               return NULL;
+       NL_MY_NAME(new) = xstrdup(my_name);
+       NL_MON_NAME(new) = xstrdup(mon_name);
  
        return new;
 }



Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.



Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Possible bugs in nfs-utils

[Neil Brown]

Thanks for the review!!

I've created this addendum patch.

NeilBrown

>From 0523fd513c6baa8dbf45d1a7afea2044262aeb3d Mon Sep 17 00:00:00 2001
From: Neil Brown <neilb@suse.de>
Date: Fri, 23 Jun 2006 17:10:56 +1000
Subject: [PATCH] Further coverity related cleanups.

Greg Banks suggested some variations, particularly improved
use of xmalloc/xstrdup functions.  Thanks.
---
 support/misc/mountpoint.c |    5 ++---
 utils/idmapd/idmapd.c     |    7 +++++--
 utils/statd/notlist.c     |   12 +++---------
 3 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/support/misc/mountpoint.c b/support/misc/mountpoint.c
index 2cf1324..750b6e8 100644
--- a/support/misc/mountpoint.c
+++ b/support/misc/mountpoint.c
@@ -22,9 +22,8 @@ is_mountpoint(char *path)
 	struct stat stb, pstb;
 	int rv;
 
-	dotdot = malloc(strlen(path)+4);
-	if (!dotdot)
-		return 0;
+	dotdot = xmalloc(strlen(path)+4);
+
 	strcat(strcpy(dotdot, path), "/..");
 	if (lstat(path, &stb) != 0 ||
 	    lstat(dotdot, &pstb) != 0)
diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
index 1231db4..5fc7811 100644
--- a/utils/idmapd/idmapd.c
+++ b/utils/idmapd/idmapd.c
@@ -1003,12 +1003,15 @@ mydaemon(int nochdir, int noclose)
 
 	if (noclose == 0) {
 		tempfd = open("/dev/null", O_RDWR);
+		if (tempfd < 0)
+			tempfd = open("/", O_RDONLY);
 		if (tempfd >= 0) {
 			dup2(tempfd, 0);
 			dup2(tempfd, 1);
 			dup2(tempfd, 2);
-		}
-		closeall(3);
+			closeall(3);
+		} else
+			closeall(0);
 	}
 
 	return;
diff --git a/utils/statd/notlist.c b/utils/statd/notlist.c
index 98aa6e2..b74d9df 100644
--- a/utils/statd/notlist.c
+++ b/utils/statd/notlist.c
@@ -54,19 +54,13 @@ nlist_new(char *my_name, char *mon_name,
 {
 	notify_list	*new;
 
-	if (!(new = (notify_list *) xmalloc(sizeof(notify_list))))
-		return NULL;
+	new = (notify_list *) xmalloc(sizeof(notify_list));
 	memset(new, 0, sizeof(*new));
 
 	NL_TIMES(new) = MAX_TRIES;
 	NL_STATE(new) = state;
-	if (!(NL_MY_NAME(new) = xstrdup(my_name))
-	    || !(NL_MON_NAME(new) = xstrdup(mon_name))) {
-		if (NL_MY_NAME(new))
-			free(NL_MY_NAME(new));
-		free(new);
-		return NULL;
-	}
+	NL_MY_NAME(new) = xstrdup(my_name);
+	NL_MON_NAME(new) = xstrdup(mon_name);
 
 	return new;
 }
-- 
1.4.0


Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs