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

"sudo -E" breaks things #257

Open
mxmilkiib opened this issue Mar 23, 2020 · 5 comments
Open

"sudo -E" breaks things #257

mxmilkiib opened this issue Mar 23, 2020 · 5 comments

Comments

@mxmilkiib
Copy link

Not sure what I've maybe done or what has changed, but I'm getting this message when pazi runs;

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', src/libcore/result.rs:1188:5
stack backtrace:
   0:     0x55e3c0c0f5e4 - <unknown>
   1:     0x55e3c0c30f1c - <unknown>
   2:     0x55e3c0c0cff7 - <unknown>
   3:     0x55e3c0c1174e - <unknown>
   4:     0x55e3c0c11441 - <unknown>
   5:     0x55e3c0c11e2b - <unknown>
   6:     0x55e3c0c119de - <unknown>
   7:     0x55e3c0c2da7e - <unknown>
   8:     0x55e3c0c2db77 - <unknown>
   9:     0x55e3c0b5911d - <unknown>
  10:     0x55e3c0b66a1c - <unknown>
  11:     0x55e3c0b6478b - <unknown>
  12:     0x55e3c0b542f3 - <unknown>
  13:     0x55e3c0c11873 - <unknown>
  14:     0x55e3c0c1997a - <unknown>
  15:     0x55e3c0c122c0 - <unknown>
  16:     0x55e3c0b6a122 - <unknown>
  17:     0x7f3cf0df4023 - __libc_start_main
  18:     0x55e3c0b360de - <unknown>
  19:                0x0 - <unknown>
@LinuxMercedes
Copy link
Collaborator

Can you check the permissions on ~/.config/pazi/pazi_dirs.msgpack and make sure you can read and write it?

@mxmilkiib
Copy link
Author

16K -rw-r--r-- 1 root root 15K Mar 23 14:05 /home/milk/.config/pazi/pazi_dirs.msgpack

Aaaah, I think it was my use of sudo -E /bin/zsh that did it.

@LinuxMercedes
Copy link
Collaborator

Oh, I bet that would do it.

At a bare minimum we can make the error a little more self-explanatory but it would be nice for sudo -E to not break things too.

@mxmilkiib mxmilkiib changed the title thread 'main' panicked at 'called Result::unwrap() ... "Permission denied" "sudo -E" breaks things Mar 23, 2020
@euank
Copy link
Owner

euank commented Mar 27, 2020

Thanks for reporting the issue and helping us dig into it 😃

That error message indeed was bad. I've made a PR that cleans up that unwrap and generally improves error handling (#258). With that pr, the error would be:

could not open pazi frecency file: "/home/user/.config/pazi/pazi_dirs.msgpack"

Caused by:
    Permission denied (os error 13)

That improves one part of this.

There could be something to do for avoiding this specific cause of wrong permissions in the first place though. The options that come to mind are:

  1. Insist this scenario isn't our problem and throw up our hands.
    The reality is, home directories and sudo are a whole thing. sudo -H -E is one way to deal with it, some distros solve this by shipping a sudoers file with always_set_home, etc etc.

  2. Detect heuristically if the data directory seems wrong
    i.e. use getent passwd $UID and compare the home directory there with $HOME... Or just special case $UID != $EUID or $UID = 0. I really don't like that, especially because what we'd really need is XDG_DATA_HOME and I don't think there's any reasonable way to get a canonical version of that .

  3. Copy permissions from the source file when we update the database
    As long as the first time you run pazi for a given HOME directory is correct, future updates to the database will have correct permissions.

For a little more context on what I mean by 3, pazi updates pazi_dirs.msgpack atomically by creating a new file (such as pazi_dirs.msgpack.123), and then renaming it over the original one. It's the process of creating that new file where permissions are changing.
We could create that file with identical permissions to the pazi_dirs.msgpack file each time.

I think 2 isn't really an option. 1 and 3 both have their points to them, but 3 seems easy enough and like it would have avoided the problem described in this issue quite neatly.

I think I've talked myself into 3 being the right option actually, so if that makes sense, I think we can implement that change to fix this

@LinuxMercedes
Copy link
Collaborator

I agree that 3 sounds like what you'd expect to happen anyway, not terribly difficult to implement, and unlikely to do the wrong thing unless you have some very unusual circumstances.

bors bot added a commit that referenced this issue Mar 29, 2020
258: *: improve error messages  r=euank a=euank



This switches things over to anyhow for many (but not all) of our
errors, and removes the use of unwrap from database loading.

Here's the before and after for an error opening the frecency database:

Before:

```
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', src/frecent_paths.rs:43:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

After:

```
could not open pazi frecency file: "/home/user/.config/pazi/pazi_dirs.msgpack"

Caused by:
    Permission denied (os error 13)
```

I think it looks much nicer with this :)

The main things that weren't switched over are the things that are using specific error variants. For those, we'll probably want to have a thiserror layer that gets wrapped in any anyhow error, and then downcast at callsites.

I want to deal with that separately.

This improves #257 in that it significantly improves the error message.

Co-authored-by: Euan Kemp <euank@euank.com>
bors bot added a commit that referenced this issue Apr 2, 2020
258: *: improve error messages  r=euank a=euank



This switches things over to anyhow for many (but not all) of our
errors, and removes the use of unwrap from database loading.

Here's the before and after for an error opening the frecency database:

Before:

```
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', src/frecent_paths.rs:43:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

After:

```
could not open pazi frecency file: "/home/user/.config/pazi/pazi_dirs.msgpack"

Caused by:
    Permission denied (os error 13)
```

I think it looks much nicer with this :)

The main things that weren't switched over are the things that are using specific error variants. For those, we'll probably want to have a thiserror layer that gets wrapped in any anyhow error, and then downcast at callsites.

I want to deal with that separately.

This improves #257 in that it significantly improves the error message.

Co-authored-by: Euan Kemp <euank@euank.com>
bors bot added a commit that referenced this issue Apr 7, 2020
258: *: improve error messages  r=euank a=euank



This switches things over to anyhow for many (but not all) of our
errors, and removes the use of unwrap from database loading.

Here's the before and after for an error opening the frecency database:

Before:

```
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', src/frecent_paths.rs:43:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

After:

```
could not open pazi frecency file: "/home/user/.config/pazi/pazi_dirs.msgpack"

Caused by:
    Permission denied (os error 13)
```

I think it looks much nicer with this :)

The main things that weren't switched over are the things that are using specific error variants. For those, we'll probably want to have a thiserror layer that gets wrapped in any anyhow error, and then downcast at callsites.

I want to deal with that separately.

This improves #257 in that it significantly improves the error message.

Co-authored-by: Euan Kemp <euank@euank.com>
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

No branches or pull requests

3 participants