Skip to content

uucore: fix order of group IDs return from entries::get_groups()#2371

Merged
tertsdiepraam merged 1 commit intouutils:masterfrom
jhscheer:fix_get_groups
Jun 8, 2021
Merged

uucore: fix order of group IDs return from entries::get_groups()#2371
tertsdiepraam merged 1 commit intouutils:masterfrom
jhscheer:fix_get_groups

Conversation

@jhscheer
Copy link
Contributor

@jhscheer jhscheer commented Jun 7, 2021

As discussed here: #2361
the group IDs returned for GNU's 'group' and GNU's 'id --groups'
starts with the effective group ID.
This implements a wrapper for entris::get_groups() which mimics
GNU's behaviour.

  • add tests for id
  • add tests for groups
  • fix id --groups --real to no longer ignore --real

Copy link
Collaborator

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Were you able to confirm that this solves the issue? Code-wise, I just have some small nits. Also there's this section from the libc manpages:

It is unspecified whether the effective group ID of the calling process is included in the returned list. (Thus, an application should also call getegid(2) and add or remove the resulting value.)

So I think the egid should always be added, even when it's not in the list? Would that be the desired behaviour?

@jhscheer
Copy link
Contributor Author

jhscheer commented Jun 7, 2021

Nice! Were you able to confirm that this solves the issue?

On my local hosts, yes. So far some CI tests don't look happy, I have to investigate that further.

Also there's this section from the libc manpages:

It is unspecified whether the effective group ID of the calling process is included in the returned list. (Thus, an application should also call getegid(2) and add or remove the resulting value.)

So I think the egid should always be added, even when it's not in the list? Would that be the desired behaviour?

I also came across that note in the man pages, but wasn't too sure what to make of it.

@tertsdiepraam
Copy link
Collaborator

Alright, then we should probably just keep it like this

@tertsdiepraam
Copy link
Collaborator

tertsdiepraam commented Jun 7, 2021

Locally, I also get this:

---- test_groups::test_groups stdout ----
current_directory_resolved: 
run: /home/terts/Programming/coreutils/target/debug/coreutils groups
current_directory_resolved: 
run: groups
thread 'test_groups::test_groups' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
<"terts sys network power lp wheel\n"
>"sys network power lp wheel terts\n"

', tests/common/util.rs:206:9

Which I think happens because my groups command is not coreutils but shadow-utils, apparently. Maybe something similar is going on in the CI?

@jhscheer jhscheer marked this pull request as draft June 8, 2021 06:50
@tertsdiepraam
Copy link
Collaborator

Maybe this helps. The OpenBSD implementation does this to construct the list of groups: https://github.com/openbsd/src/blob/fdf1c64914f5ad56fb382b8fd0e46398bc71a4cf/usr.bin/id/id.c#L305-L311

@jhscheer
Copy link
Contributor Author

jhscheer commented Jun 8, 2021

Maybe this helps. The OpenBSD implementation does this to construct the list of groups: https://github.com/openbsd/src/blob/fdf1c64914f5ad56fb382b8fd0e46398bc71a4cf/usr.bin/id/id.c#L305-L311

Thanks. That's BSD licensed right? I'll take a look if I can't figure this out otherwise.

@tertsdiepraam
Copy link
Collaborator

tertsdiepraam commented Jun 8, 2021

Yeah, I think we can look at it because it's BSD licensed

@jhscheer jhscheer force-pushed the fix_get_groups branch 3 times, most recently from a45439d to e5428f8 Compare June 8, 2021 09:46
@jhscheer
Copy link
Contributor Author

jhscheer commented Jun 8, 2021

Also there's this section from the libc manpages:

It is unspecified whether the effective group ID of the calling process is included in the returned list. (Thus, an application should also call getegid(2) and add or remove the resulting value.)

So I think the egid should always be added, even when it's not in the list? Would that be the desired behaviour?

I also came across that note in the man pages, but wasn't too sure what to make of it.

I implemented this:

As implied by the definition of supplementary groups, the
effective group ID may appear in the array returned by
getgroups() or it may be returned only by getegid(). Duplication
may exist, but the application needs to call getegid() to be sure
of getting all of the information. Various implementation
variations and administrative sequences cause the set of groups
appearing in the result of getgroups() to vary in order and as to
whether the effective group ID is included, even when the set of
groups is the same (in the mathematical sense of ``set''). (The
history of a process and its parents could affect the details of
the result.)
(From: https://www.man7.org/linux/man-pages/man3/getgroups.3p.html)

Everything looks good now :)

@jhscheer jhscheer marked this pull request as ready for review June 8, 2021 10:14
Copy link
Collaborator

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Just some small nits and questions

As discussed here: uutils#2361
the group IDs returned for GNU's 'group' and GNU's 'id --groups'
starts with the effective group ID.
This implements a wrapper for `entris::get_groups()` which mimics
GNU's behaviour.

* add tests for `id`
* add tests for `groups`
* fix `id --groups --real` to no longer ignore `--real`
@tertsdiepraam
Copy link
Collaborator

tertsdiepraam commented Jun 8, 2021

Actually, I have one more request (sorry). Would you mind changing that call to GNU groups into a call to id -Gn? It should be identical [1] but works on Arch-based systems, where groups is not GNU. Or is there a specific reason it needs to be util_name!? In that case, we could keep it like this.

[1]: https://www.gnu.org/software/coreutils/manual/html_node/groups-invocation.html#groups-invocation

@jhscheer
Copy link
Contributor Author

jhscheer commented Jun 8, 2021

Actually, I have one more request (sorry). Would you mind changing that call to GNU groups into a call to id -Gn? It should be identical 1 but works on Arch-based systems, where groups is not GNU. Or is there a specific reason it needs to be util_name!? In that case, we could keep it like this.

I think if you want to tinker with test_groups_username, it would be better not to do it in this PR.
Should I remove this test and just keep the TODO comments?

@tertsdiepraam
Copy link
Collaborator

Right, yeah I see that's non-trivial. Since it's just my machine I'll make a separate PR. You don't have to change this one

@tertsdiepraam tertsdiepraam merged commit 05f65b0 into uutils:master Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants