Panic with "unknown userid" and "runtime error: invalid memory address or nil pointer dereference"

[leira]

I installed lf through nix on gLinux, which is a debian based distribution within Google. When I launched lf, it panic'ed and printed:

2019/06/24 14:45:28 user: user: unknown userid 674665
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x40 pc=0x56b17f]

goroutine 1 [running]:
main.init.3()
        /build/source/os.go:71 +0x7df

It seems it was triggered by the user.Current() call on os.go:57. It might be an error only happens with gLinux, as gLinux use a different way to authenticate users, I couldn't find an entry of my user in /etc/passwd. I will try to dig it further when I have time. ranger worked fine through.

I'm not familiar with golang through, it seems lf only uses gUser = user.Current() to get HomeDir and UserName. It did have $HOME and $USER check around os.go:60, but it didn't use them. Maybe refactor gUser into gHomeDir and gUserName, and take the values from $HOME and $USER when user.Current() fails? Or at least fail on the spot rather than trigger a panic later on?

[gokcehan]

@leira Looking at lookup_stubs.go I feel like it should have already use $HOME and $USER variables as the fallback to fill gUser but somehow it shows up as nil. Those checks in our os.go:60 and os.go:63 are only added to have a more informative error message for users. Do you happen to know if user.Current() ever works in gLinux?

As far as I understand, nix builds from source. Can you make sure you have a relatively new go installation? I think they changed something in os/user package a while ago. Maybe you can try an appropriate prebuilt binary to see if it works. Also I think nix repo has the version r10 whereas our latest version is r12 though I don't think we have changed anything about this recently.

[gokcehan]

@leira Closing this issue as unfortunately there is no way for me to reproduce or solve it on my own. Feel free to add further feedback on the issue if you like.

[leira]

@gokcehan I didn't have a chance to revisit the issue yet. Thanks for closing it. I will reopen it when I have more information.

[danieldk]

Ran into this as well. The issue can be reproduced by running lf in a bubblewrap sandbox where /etc/passwd is not visible. E.g. (on NixOS):

bwrap --ro-bind /nix /nix --dev /dev `realpath results/lf/bin/lf`

The problem is that gUser is dereferenced here:

lf/os.go

Line 72 in 2b445aa

config = filepath.Join(gUser.HomeDir, ".config")

even when when user.Current() returns nil. Maybe the code should panic or exit with a friendly message when user.Current() returns nil?

Anyway, it's probably not a big issue, not a lot of people will be running something on Linux without /etc/passwd available.

[gokcehan]

@danieldk Thanks for the report.

It looks like there has been changes in lookup_stubs.go the last time I visit this issue. It seems that our manual check for HOME and USER are not necessary anymore since they are integrated into the implementation in the standard library. Looking at the code here again, it should fall back to USER variable when /etc/passwd file is not available.

I'm not familiar with nix. Does it install our prebuilt-binaries or build the package from the source? If latter, which version of Go are you using? For prebuilt-binaries, it seems that we're still using the default Go version in Travis, which is Go1.11. Fallback behavior may not have been available in this version. I guess maybe it is a good idea to bump the go version in Travis, though I don't like messing around with the build system.

Go team already seems to be interested in covering corner cases. In the long term, I think we should simply change our code to call user.Current() and fail when err != nil and then report other cases if any to the Go team. Reopening the issue in the meantime.

[danieldk]

I'm not familiar with nix. Does it install our prebuilt-binaries or build the package from the source? If latter, which version of Go are you using?

We build from source, currently using Go 1.14.x.

Go team already seems to be interested in covering corner cases. In the long term, I think we should simply change our code to call user.Current() and fail when err != nil and then report other cases if any to the Go team. Reopening the issue in the meantime.

Sounds good! I think it is perfectly reasonable to fail when err != nil, not having /etc/passwd is very rare. But I think a better error message would go a long way for people who bump into this.

[Patagonicus]

Hi all,

While I don't use lf myself (yet? I saw it for the first time yesterday), I got pointed to this bug. And because I like Nix(OS) and Go and had a few minutes to spare, I wrote a hacky fix of this to make it work in bubblewrap and on gLinux. It just uses $HOME and $USER if it can't find the user through the stdlib and as far as I can tell that's all that's used from the object.

I've also wrapped that into a self-contained Nix overlay, so if you're using Nix, you can just put overlays.nix in ~/.config/nixpkgs (for NixOS, add nixpkgs.overlays = followed by the contents of the file, I think).

The patch and overlays.nix are here: https://gist.github.com/Patagonicus/56977ce9bb3ce4a8d1ba536214aef4c1

They are based on the commit at tag r20 because that's what nixpkgs was using for the package at the time I was looking at it. Like I said above, this is hacky, so I wouldn't want this to be merged as is, but at least it should let people use lf on gLinux for now. :)

Cheers,
Philipp

[GaetanLepage]

Hello !
I would like to know if this will this be fixed upstream ?

Thank you for developing lf !

[GaetanLepage]

Would it be relevant for me to open a PR on this repo with @Patagonicus's patch ?
Indeed, the best would be to fix this upstream.

What do you think @gokcehan ?

[davewongillies]

FWIW, I only get this error from the version of lf installed via homebrew. If I install it using go install its fine

[gokcehan]

@GaetanLepage Sorry for late reply as I don't look at issues often nowadays.

Yes, if you send a PR I can merge it. At this point I don't care how it is supposed to work and why it is not working so it is better to fix it upstream here.