Comment 3 for bug 1787548

Hi Joe,
thank you very much for such a swift response and fixing the bug!

And also for the very useful explanation, which let's me better understand the patch.
I'm sorry though, I won't be able to test your patch (I'm traveling to a conference and I won't be able to setup my system to compile fscrypt from source soon). But I bet you've tested it well.

If you don't mind, I have some comments to the code/patch (bear with me, I don't speak GO and know the code of pam-fscrypt only from this patch, so I may be completely stupid..)

> // StopAsPamUser restores the original privileges that were running the
 // PAM module (this is usually root). As this error is often ignored in a defer
 // statement, any error is also logged.
 func (h *Handle) StopAsPamUser() error {
- err := security.SetProcessPrivileges(h.OrigUser)
+ err := security.SetProcessPrivileges(h.origPrivs)
  if err != nil {
   log.Print(err)
  }

I'm surprised by the comment "this err is often ignored..", as this seems the critical part (returning to orig privileges). Although it returns error(?) so a failure is probably handeled elsewhere on a higher layer. (Actually, how is this handeled? - if there's an err in privilege escalation, it's ok, you'd just say "sorry, fscrypt failed to run"; but if deescalation fails - what'd you do? Kill the process, kernel panic?)

> + n, err := C.getgroups(C.int(len(groups)), &groups[0])
+ if n < 0 {
+ return nil, err
+ }

Couple of things here, although likely just nitpiks.
You index groups w/o checking for size>0, although I wasn't able to create a user who is not in atleast 1 grp. maybe the make() can fail?
if could be if n < 1: error;
on the line "n, err :=" you don't check for err, you do it everywhere else. But probably getgroups cannot fail and only returns n>0 if no err?

> +// specified by privs. The original privileges can be by first saving the output
typo: can be ..obtained?..by

The patch looks like a nice cleanup that simplifies things.
Thank you for your work on the project!
cheers, Mark