macports.tcl: doesn't reset user if svn fails

[mojca (Mojca Miklavec)]

When svn fails to sync the ports (in my case that's because it doesn't accept the certificate), rsync continues to run as a local user. I'm attaching an example of a patch, but please check it carefully first as I don't have no experience messing up with base.

Maybe something similar is needed at other places as well.

(The patch is against trunk, but it would be nice to fix this for 2.3.0 as well.)

[mojca (Mojca Miklavec)]

a patch to set user back to root even when svn fails

[mojca (Mojca Miklavec)]

One way to test this it to add one extra debug line with geteuid to /opt/local/share/macports/Tcl/macports1.0/macports.tcl:

set needs_portindex 0
ui_info "Synchronizing local ports tree from $source"
ui_info ">>> Hello me ([geteuid])"

and to replace

system $svn_commandline

with

system "/usr/bin/false"

[neverpanic (Clemens Lang)]

Thanks, that also was a problem for git. r118570. Should we backport that into 2.3 aswell?

[mojca (Mojca Miklavec)]

Thank you. As already mentioned it would be nice to see it in 2.3.0, but that's up to you or others to decide.

I forgot to say earlier (and maybe I could write a patch as well), but it would probably be helpful to add a bit of extra ui_debug info. For example the following:

set euid [geteuid]
set egid [getegid]
ui_debug "changing euid/egid - current euid: $euid - current egid: $egid"
setegid [name_to_gid [file attributes $portdir -group]]
seteuid [name_to_uid [file attributes $portdir -owner]]

would be more helpful if both initial and final UID and GID were reported (ideally also with username, not just number?). And it would probably be nice to cleary report the the change during reversal as well.

An alternative would be to change all reports like

ui_debug $svn_commandline

into something like

ui_debug "Running '$svn_commandline' as [geteuid]"

but that would basically mean modifying debug all commands. Maybe that's too much.

What do you think of moving ui_debug $svn_commandline from before the loop to just before it gets executed, so that UID change would be reported before it is printed out?

[neverpanic (Clemens Lang)]

Replying to mojca@…:

Thank you. As already mentioned it would be nice to see it in 2.3.0, but that's up to you or others to decide.

I'm not in any way more qualified to decide whether to backport that to 2.3.0 than you are. In fact, you could just go ahead and cherry-pick the change onto the 2.3 branch.

I forgot to say earlier (and maybe I could write a patch as well), but it would probably be helpful to add a bit of extra ui_debug info. For example the following:

set euid [geteuid]
set egid [getegid]
ui_debug "changing euid/egid - current euid: $euid - current egid: $egid"
setegid [name_to_gid [file attributes $portdir -group]]
seteuid [name_to_uid [file attributes $portdir -owner]]

would be more helpful if both initial and final UID and GID were reported (ideally also with username, not just number?). And it would probably be nice to cleary report the the change during reversal as well.

Actually I was surprised the change was done manually. There are a few other places in MacPorts where euid/egid are set and those are IIRC capsuled in a proc that also prints some debugging info. Maybe re-using that code and making it print nicer messages would be the way to go?

In fact, one could re-factor the whole thing as a block statement with appropriate error handling, like registry::write { code } does, something along the lines of

runAsUser $uid $gid {
  # run svn command here
}

that would set euid/egid, print debug messages, run the block of code with the changed privileges and restore the previous uid/gid when the block finishes or throws an error.