Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fallback for Path.home on Unix #11544

Merged

Conversation

HertzDevil
Copy link
Contributor

Defaults to the home directory of the real user (as opposed to effective) when $HOME is empty or null. This is equivalent to:

require "system/user"

lib LibC
  fun getuid : UidT
end

Path.new(System::User.find_by(id: LibC.getuid.to_s).home_directory)

but without constructing a System::User.

@straight-shoota
Copy link
Member

Why real instead of effective? When I'm operating as an effective user, I would expect the effective user's home path.

@HertzDevil
Copy link
Contributor Author

But why? I wouldn't expect the home directory to suddenly become that of the Crystal program's owner depending on whether the program is setuid or not.

For reference, Ruby uses getpwnam(getlogin()) followed by getpwuid(getuid()).

@HertzDevil
Copy link
Contributor Author

Another reason: starting a setuid program alone will not change the HOME environment variable, thus $HOME is still associated with the real user, so the fallback should be consistent with this default behaviour.

@straight-shoota
Copy link
Member

Yeah, you're probably right. Seeing a different home directory would be weir. You are often not even aware that a program runs with a different effective user.

I was thinking that file ownership goes to the effective user, so it would be weird if they wouldn't have write permission in the home directory. But considering that effective user typically has elevated rights, it's not that relevant.

Comment on lines 17 to 20
if pwd_pointer
String.new(pwd.pw_dir)
else
raise RuntimeError.from_os_error("getpwuid_r", Errno.new(ret))
raise RuntimeError.from_os_error("getpwuid_r", Errno.new(ret.not_nil!))
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not exactly using the API correctly. getpwuid_r returns 0 and sets the result to a null pointer if not matching password record was found. That would result in showing a runtime error "success".
We should handle the case ret.zero? and pwd_pointer.nil? to generate an appropriate error message.

Most actual error codes are already handled by retry_with_buffer. I'm not sure if getpwuid_r can even return any of the ones that are ignored (and make it return nil): ENOENT, ESRCH, EBADF, EPERM. But having a generic fallback in cause anyone of these happens doesn't hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is a situation where getpwuid(getuid()) fails because no matching UID is found?

Copy link
Member

Choose a reason for hiding this comment

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

I understand that would be ret.zero? && pwd_pointer.nil?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not what I am asking. If I pass an arbitrary user ID, then it can happen that no matching record is found. What is a situation where the current real user ID has no matching record?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@beta-ziliani beta-ziliani added this to the 1.4.0 milestone Jan 28, 2022
@straight-shoota straight-shoota merged commit 8d2f3d6 into crystal-lang:master Jan 31, 2022
@HertzDevil HertzDevil deleted the feature/unix-path-home-fallback branch February 1, 2022 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants