[PATCH] addgroup could assign already in use group

Denys Vlasenko vda.linux at googlemail.com
Mon Feb 9 00:30:53 UTC 2015


On Sat, Sep 20, 2014 at 4:45 PM, tito <farmatito at tiscali.it> wrote:
> 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.

Unfortunately, bbox and a lot of other code has tons of similar bugs.
For example, fopen() returning NULL is usually treated as "file does
not exist", and if this was an attempts to read an optional file (e.g.
conf file...), the program happily continues, wrongly assuming that
file does not exist, whereas it can be EMFILE error.

Mass fixing this probably would add too much code obfuscation compared
to marginal gains in error handling.

In this particular case...

if you are getting EIO, addgroup not working right is not your biggest worry.

EINTR is unlikely or even impossible (we do not have any active signal
handlers, so EINTR can only be generated by e.g. subtle bugs in
ptrace).

EMFILE is possible if addgroup was deliberately run under ulimit -n 3.
Don't do that :)

and so on...


More information about the busybox mailing list