Allow setfacl / acl_from_text() to handle negative uid / gid numbers in user/group ACL entries

[pen_lysator.liu.se]

Summary

getfacl / acl_to_text() incorrectly prints uid/gid numbers as signed integers. This causes uid / gid numbers larger than 2G (2147483648) to print as negative numbers. The libc acl_from_text() function does not handle negative numbers. This diff adds a backwards compatiblity fix to allow negative numbers...

Test Plan

Make sure this works:

# mkdir testdir
# setfacl -a0 group:-1294967291:rwxpDdaARWcCos:fd-----:allow testdir

[rmacklem]

lib/libc/posix1e/acl_from_text.c

285

Although this does the trick, I don't see any
need for the if/else. Could we just always cast
it to (unsigned long).

Another words, just replace line#284 in the old
code with #285 in the new code?
(Oh, and being really nitpicky, style(9) doesn't
specify a <space> after a cast, I think?)

302

Same as above.

[rmacklem]

Oh, and I'd leave the comment (or something like it)
in the code, so that it is obvious why the cast'ng is done.

Thanks, rick

[rmacklem]

lib/libc/posix1e/acl_from_text.c

288

The second part of the conditional
"l != (unsigned long)(uid_t)l" looks
like it was intended to make "-" ids fail.

I cannot see why "-" ids shouldn't be
allowed, but it doesn't make sense to
leave this second part of the conditional
in there if we are casting it "(unsigned long)(uid_t)"
anyhow.

[pen_lysator.liu.se]

Hmm.. Yeah, I didn't want to change that part of the code. But now when I read the fine print in the man page for strtoul() and strtol() I think we should just get rid of that extra check and just use "*endp != '\0'"...

Hmm.. Shouldn't we use strtoumax() and strtoimax() while we're at it...? Perhaps something like:

intmax_t iv;
uintmax_t uv;
uid_t id;
...
if (*name == -) {
        iv = strtoimax(name,&endp,0);
} else {
        uv = strtoumax(name,&endp,0);
}
if (name == endp || *endp != '\0') {
        errno = EINVAL; /* No or invalid number */
        return (-1);
}
if (*name == '-') {
        id = iv;
        if (int32_t) id != iv) {
               errno = EINVAL; /* Overflow */
               return (-1);
       }
} else {
        id = uv;
        if (id != uv) {
                errno = EINVAL; /* Overflow */
                return (-1);
        }
}

Hmm.. Not sure (or I'm too tired right now) how to write a better and fully portably check for under/overflow in the negative case that works if we get 64 bit uid_t/gid_t. :-)

[rmacklem]

In D57180#1310528, @pen_lysator.liu.se wrote:

Hmm.. Yeah, I didn't want to change that part of the code. But now when I read the fine print in the man page for strtoul() and strtol() I think we should just get rid of that extra check and just use "*endp != '\0'"...

Hmm.. Shouldn't we use strtoumax() and strtoimax() while we're at it...? Perhaps something like:

intmax_t iv;
uintmax_t uv;
uid_t id;
...
if (*name == -) {
        iv = strtoimax(name,&endp,0);
} else {
        uv = strtoumax(name,&endp,0);
}
if (name == endp || *endp != '\0') {
        errno = EINVAL; /* No or invalid number */
        return (-1);
}
if (*name == '-') {
        id = iv;
        if (int32_t) id != iv) {
               errno = EINVAL; /* Overflow */
               return (-1);
       }
} else {
        id = uv;
        if (id != uv) {
                errno = EINVAL; /* Overflow */
                return (-1);
        }
}

Hmm.. Not sure (or I'm too tired right now) how to write a better and fully portably check for under/overflow in the negative case that works if we get 64 bit uid_t/gid_t. :-)

Since "long" is already 64bits on most arches (except armv7?),
I think leaving it as "long" is fine.

As for allowing a negative value, but catching overflows,
I think the easiest way is to define a long variable,
such as l0 and then changing the code to:

long l0; /* added for range check */

l = l0 = strtoul(name, &endp, 0);
if (*endp != '\0' || l0 < INT32_MIN || l0 > UID_MAX)

This seems to work for a small test program.
(I'll admit I came up with a couple of incorrect ones
before I tried this straightforward version.)
There isn't any UID_MIN defined, but limiting it
to INT32_MIN seems reasonable to me?

lib/libc/posix1e/acl_from_text.c

288

The second part of the conditional
"l != (unsigned long)(uid_t)l" looks
like it was intended to make "-" ids fail.

I cannot see why "-" ids shouldn't be
allowed, but it doesn't make sense to
leave this second part of the conditional
in there if we are casting it "(unsigned long)(uid_t)"
anyhow.

Yea, ignore my above comment. I now realize the
current code catches overflows of uid_t.

[pen_lysator.liu.se]

Better handling of over/underflow and cleaner code..

[rmacklem]

lib/libc/posix1e/acl_from_text.c

272

You could make this a "static bool", I think?
(You'll need to add #include <stdbool.h>.)