possible uid/gid confusion

[robertlemmen]

while trying to understand build failures with libuv 1.30.1, I came across this (unrelated to the failure) code in src/io/fileops.c:

#define FILE_IS(name, rwx) \
    MVMint64 MVM_file_is ## name (MVMThreadContext *tc, MVMString *filename, MVMint32 use_lstat) { \
        if (!MVM_file_exists(tc, filename, use_lstat)) \
            return 0; \
        else { \
            uv_stat_t statbuf = file_info(tc, filename, use_lstat); \
            MVMint64 r = (statbuf.st_mode & S_I ## rwx ## OTH) \
                      || (statbuf.st_uid == geteuid() && (statbuf.st_mode & S_I ## rwx ## USR)) \
                      || (statbuf.st_uid == getegid() && (statbuf.st_mode & S_I ## rwx ## GRP)); \
            return r ? 1 : 0; \
        } \
    }

I am pretty certain that the comparison with getegid() should be with statbuf.st_gid

[dogbert17]

Fixed with fc78a13

[patrickbkr]

I honestly don't understand either the new or the old code.
What I understand the code does currently is compare the users group to the files group.

My understanding of groups in Linux:

• The user group is just the group new files are assigned when the user creates them
• To check whether a user has permissions to do something with a file one needs to compare the user to all the users that are in the files group

Am I totally confused?
@dogbert17, @robertlemmen

[robertlemmen]

My understanding of how this works is slightly different, a simple example:

ls -l
-rw-rw-r-- 1 robertle devgrp 213 Sep 22 19:14 Makefile

so this file is owned by user 'robertle' and group 'devgrp'. permissions for the user are 'rw-', for the group 'rw-' and for all other users 'r--'. so it would be writable by 'robertle', which is what the statbuf.st_uid == geteuid() line checks for.

it is also writable by all users in the 'devgrp' group, I suspect that's what the getegid line is trying to check, but it does compare to the user-owner of the file, which seems like a (now fixed) bug.

you are correct though that getegid() returns one group id, but the caller could be in multiple groups. so instead of getegid() we should use group_member() or something like that!

[patrickbkr]

@robertlemmen Can you have a look at whether PR #1193 fixed this issue completely?

[robertlemmen]

the PR looks good to me, and fixes the one problem I could see. but I never had a full set of tests or any way to really verify the behavior...