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.c23 Nov 2013 17:14:05 -  1.98
+++ src/usr.sbin/user/user.c30 Dec 2013 15:58:48 -
@@ -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);
 

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

[Ted Unangst]

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 -  1.98
> +++ src/usr.sbin/user/user.c  30 Dec 2013 15:58:48 -
> @@ -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.