user(8) free() before returning in groupmod()

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

user(8) free() before returning in groupmod()

Loganaden Velvindron
From NetBSD:

Coverity annotation -- although memsave free()s its first argument, it
    will allocate memory and assign it to its first argument, so it is neutral

Coverity CID 3228:  memory leak -- failed to free() newname in groupmod()

Index: src/usr.sbin/user/user.c
===================================================================
RCS file: /cvs/src/usr.sbin/user/user.c,v
retrieving revision 1.98
diff -u -p -r1.98 user.c
--- src/usr.sbin/user/user.c 23 Nov 2013 17:14:05 -0000 1.98
+++ src/usr.sbin/user/user.c 30 Dec 2013 15:58:48 -0000
@@ -2230,7 +2230,8 @@ groupmod(int argc, char **argv)
  cc = strlcat(buf, "\n", sizeof(buf));
  if (cc >= sizeof(buf))
  errx(EXIT_FAILURE, "group `%s' entry too long", grp->gr_name);
-
+ if (newname != NULL)
+ free(newname);
  openlog("groupmod", LOG_PID, LOG_USER);
  if (!modify_gid(*argv, buf))
  err(EXIT_FAILURE, "can't change %s file", _PATH_GROUP);
 

Reply | Threaded
Open this post in threaded view
|

Re: user(8) free() before returning in groupmod()

Ted Unangst-6
On Mon, Dec 30, 2013 at 08:23, Loganaden Velvindron wrote:

> Index: src/usr.sbin/user/user.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/user/user.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 user.c
> --- src/usr.sbin/user/user.c 23 Nov 2013 17:14:05 -0000 1.98
> +++ src/usr.sbin/user/user.c 30 Dec 2013 15:58:48 -0000
> @@ -2230,7 +2230,8 @@ groupmod(int argc, char **argv)
> cc = strlcat(buf, "\n", sizeof(buf));
> if (cc >= sizeof(buf))
> errx(EXIT_FAILURE, "group `%s' entry too long", grp->gr_name);
> -
> + if (newname != NULL)
> + free(newname);
> openlog("groupmod", LOG_PID, LOG_USER);
> if (!modify_gid(*argv, buf))
> err(EXIT_FAILURE, "can't change %s file", _PATH_GROUP);

This is also poor style. There's no need to check for NULL.