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

fix: use HOME dir if set #2177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: use HOME dir if set #2177

wants to merge 1 commit into from

Conversation

dpc
Copy link

@dpc dpc commented Apr 24, 2024

$HOME should be preferred source of home directory information.

It's a common pattern to run software with a custom HOME to temporarily isolate it from the system-set home directory. All Unix software I ever worked with would respect HOME.

Preferring user.Current() (which I guess reads from /etc/passwd) make it impossible to temporarily change effective home, as changing /etc/passwd is not an option.

I'm currently migrating things to a self-hosted github-actions runner, where one system user is used for all github actions runners, but each instance gets a different HOME, and is otherwise sandboxed to not even have permissions to touch the original home. Everything works except lncli which fails with:

[lncli] could not load global options: could not read profile file /home/github-runner/.lncli/profiles.json: could not load profile file /home/github-runner/.lncli/profiles.json: open /home/github-runner/.lncli/profiles.json: permission denied

and I traced it to this code.

HOME directory should be preferred source of home directory information.

It's a common pattern to run software with a custom HOME to temporarily isolate it from the system-set home directory. All Unix software I ever worked with would respect HOME.

Preferring `user.Current()` (which I guess reads from `/etc/passwd`) make it impossible to temporarily change effective home, as changing `/etc/passwd` is not an option.

I'm currently migrating things to a self-hosted github-actions runner, where one system user is used for all github actions runners, but each instance gets a different `HOME`, and is otherwise sandboxed to not even have permissions to touch the original home. Everything works except `lncli` which fails with:

```
[lncli] could not load global options: could not read profile file /home/github-runner/.lncli/profiles.json: could not load profile file /home/github-runner/.lncli/profiles.json: open /home/github-runner/.lncli/profiles.json: permission denied
```

and I traced it to this code.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 8811320930

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.008%) to 56.878%

Changes Missing Coverage Covered Lines Changed/Added Lines %
btcutil/appdata.go 4 5 80.0%
Files with Coverage Reduction New Missed Lines %
peer/peer.go 6 74.16%
Totals Coverage Status
Change from base Build 8802845904: -0.008%
Covered Lines: 29452
Relevant Lines: 51781

💛 - Coveralls

@guggero
Copy link
Collaborator

guggero commented Apr 29, 2024

Thanks for the PR. But I'm very hesitant to accept a change like this, since it seems it would change the default behavior of all our projects that rely on this code. I'd say it's very unusual for user.Current() to differ from $HOME.
I understand your case, but shouldn't the actual fix here be that lncli doesn't error out if it cannot read a file that it's not using anyway? Since the profiles.json is just ignored when it doesn't exist, so it should also ignore if the path cannot be accessed.

@dpc
Copy link
Author

dpc commented Apr 29, 2024

But I'm very hesitant to accept a change like this, since it seems it would change the default behavior of all our projects that rely on this code.

Yeah. This is a valid concern.

I'd say it's very unusual for user.Current() to differ from $HOME.

That's subjective I guess, but I don't find it uncommon, and use that functionality a lot. It's especially common when dealing with systemd units, long running daemons, etc. Thought I guess the fact that I'm the first to report it, might suggest that you're right. But by the same coin - not a lot of people will be affected by switching it.

As far as I know HOME is the way to get home directory. I did some experiments and it seems like e.g. all programming languages I tried will use HOME first and then fallback to getpwuid_r.

Rust standard library:

https://doc.rust-lang.org/std/env/fn.home_dir.html

Python:

> env HOME=/tmp python
Python 3.11.8 (main, Feb  6 2024, 21:21:21) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from pathlib import Path
>>> print(Path.home())
/tmp
>>>

> env -u HOME python
Python 3.11.8 (main, Feb  6 2024, 21:21:21) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from pathlib import Path
>>> print(Path.home())
/home/dpc
>>>

Node.js:

> env HOME=/tmp nix run nixpkgs#nodejs
warning: $HOME ('/tmp') is not owned by you, falling back to the one defined in the 'passwd' file ('/home/dpc')
Welcome to Node.js v20.12.2.
Type ".help" for more information.
>  require('os').homedir()
'/tmp'
>

> env -u HOME nix run nixpkgs#nodejs
Welcome to Node.js v20.12.2.
Type ".help" for more information.
> require('os').homedir()
'/home/dpc'
>

Ruby:

> env HOME=/tmp nix shell nixpkgs#ruby -c irb
warning: $HOME ('/tmp') is not owned by you, falling back to the one defined in the 'passwd' file ('/home/dpc')
irb(main):001:0> Dir.home
=> "/tmp"
irb(main):002:0>

> env -u HOME nix shell nixpkgs#ruby -c irb
irb(main):001:0> Dir.home
=> "/home/dpc"
irb(main):002:0>

Julia:

> env HOME=/tmp nix run nixpkgs#julia
warning: $HOME ('/tmp') is not owned by you, falling back to the one defined in the 'passwd' file ('/home/dpc')
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.10.2 (2024-03-01)
 _/ |\__'_|_|_|\__'_|  |
|__/                   |

julia> homedir()
"/tmp"


> env -u HOME nix run nixpkgs#julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.10.2 (2024-03-01)
 _/ |\__'_|_|_|\__'_|  |
|__/                   |

julia> homedir()
"/home/dpc"

julia>

Haskell:

> env HOME=/tmp nix shell nixpkgs#ghc -c ghci
warning: $HOME ('/tmp') is not owned by you, falling back to the one defined in the 'passwd' file ('/home/dpc')
GHCi, version 9.6.4: https://www.haskell.org/ghc/  :? for help
ghci> import System.Directory
ghci>
ghci> main :: IO ()

<interactive>:3:1: error: [GHC-88464]
    Variable not in scope: main :: IO ()
    Suggested fix: Perhaps use ‘min’ (imported from Prelude)
ghci> main = do

<interactive>:4:8: error: [GHC-82311]
    Empty 'do' block
    Suggested fix: Perhaps you intended to use NondecreasingIndentation
ghci>   homeDir <- getHomeDirectory
ghci>   putStrLn ("Your home directory is: " ++ homeDir)
Your home directory is: /tmp
ghci>


> env -u HOME nix shell nixpkgs#ghc -c ghci
GHCi, version 9.6.4: https://www.haskell.org/ghc/  :? for help
ghci> import System.Directory
ghci>
ghci> main :: IO ()

<interactive>:3:1: error: [GHC-88464]
    Variable not in scope: main :: IO ()
    Suggested fix: Perhaps use ‘min’ (imported from Prelude)
ghci> main = do

<interactive>:4:8: error: [GHC-82311]
    Empty 'do' block
    Suggested fix: Perhaps you intended to use NondecreasingIndentation
ghci>   homeDir <- getHomeDirectory
ghci>   putStrLn ("Your home directory is: " ++ homeDir)
Your home directory is: /home/dpc
ghci>

@guggero
Copy link
Collaborator

guggero commented Apr 29, 2024

Hmm, interesting. Thanks for the additional info and the test cases.

Maybe we should instead do what the shell does and check if we can actually access the directory returned by user.Current()? And if that fails, fall back to $HOME?
That would only change the default behavior if the permissions are set similar to your restricted GitHub runner case.

@dpc
Copy link
Author

dpc commented Apr 29, 2024

I understand the worry about not breaking lots of downstream projects, but I don't think making things more complicated is going to be helpful.

How about phasing it in, so:

  • compare HOME and Current(), if they are the same no worries
  • if they are different, check some BTCD_PREFER_HOME_ENV
    • if set, just use HOME
    • if not, print a warning that previous versions used technically wrong (Current()) directory, and this will change in the future. Setting BTCD_PREFER_HOME_ENV=1 allows opting-in into a new behavior and avoiding this warning

The end goal is using HOME. For time being old behavior is retained out of caution. I (and others like me) can just set BTCD_PREFER_HOME_ENV=1 and get the desired behavior. Everyone else gets warned about the change, can look it up and complain if they want to. :D

After some time old behavior and BTCD_PREFER_HOME_ENV can be removed. Only the handful of people that actually play HOME-setting shenanigans will ever know.

@guggero
Copy link
Collaborator

guggero commented Apr 30, 2024

Sounds good to me.

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.

None yet

3 participants