[PATCH] addgroup could assign already in use group

[tito]

Hi,
just a ping.

Ciao,
Tito

On Saturday 20 September 2014 16:45:14 tito wrote:
> Hi,
> while looking at the long username stuff and malloced getpwxx functions
> I've noticed a bug in addgroup resulting in assignement of a wrong
> (already in use) gid. In the function xgroup_study:
> 
> static void xgroup_study(struct group *g)
> {
> 
> snip
> 
> 	/* Check if the desired gid is free
> 	 * or find the first free one */
> 	while (1) {
> 		if (!getgrgid(g->gr_gid)) {
> 
> 
> 			return; /* found free group: return */
> 		}
> snip
> 
> 		g->gr_gid++;
> 	}
> }
> 
> The call to getgrgid can return NULL also in case of error:
> 
>        0 or ENOENT or ESRCH or EBADF or EPERM or ...
>               The given name or gid was not found.
> 
>        EINTR  A signal was caught.
> 
>        EIO    I/O error.
> 
>        EMFILE The maximum number (OPEN_MAX) of files was open already in the calling process.
> 
>        ENFILE The maximum number of files was open already in the system.
> 
>        ENOMEM Insufficient memory to allocate group structure.
> 
>        ERANGE Insufficient buffer space supplied.
> 
> a simple fix would be:
> 
> 	while (1) {
> -		if (!getgrgid(g->gr_gid)) {
> +		if (!getgrgid(g->gr_gid) && errno == 0) {
> 
> which is tested and works with glibc but this is not optimal because as man page says:
> 
> "Experiments on various UNIX-like  systems shows that lots of different values occur in this situation:"  (entry is not found)
> "0, ENOENT, EBADF, ESRCH, EWOULDBLOCK, EPERM and probably others."
> 
> A better solution would be to use getgrgid_r but this increases binary size:
> 
>  diff -uNp loginutils/addgroup.c.original loginutils/addgroup.c
> --- loginutils/addgroup.c.original      2014-08-13 13:56:04.000000000 +0200
> +++ loginutils/addgroup.c       2014-09-20 16:32:44.193586505 +0200
> @@ -32,7 +32,10 @@
>  static void xgroup_study(struct group *g)
>  {
>         unsigned max = CONFIG_LAST_ID;
> -
> +       struct group  *gr;
> +       struct group  grp;
> +       char buffer[256];
> +
>         /* Make sure gr_name is unused */
>         if (getgrnam(g->gr_name)) {
>                 bb_error_msg_and_die("%s '%s' in use", "group", g->gr_name);
> @@ -53,8 +56,9 @@ static void xgroup_study(struct group *g
>         /* Check if the desired gid is free
>          * or find the first free one */
>         while (1) {
> -               if (!getgrgid(g->gr_gid)) {
> -                       return; /* found free group: return */
> +               if (getgrgid_r(g->gr_gid, &grp, buffer, 256, &gr) == 0) {
> +                       if (gr == NULL)
> +                               return; /* found free group: return */
>                 }
>                 if (option_mask32 & OPT_GID) {
>                         /* -g N, cannot pick gid other than N: error */
> 
> 
> Attached is a patch with this solution if you think it is worth the effort and size
> increase to fix this corner case.
> 
> Ciao,
> Tito
>