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

Config and/or state files in read-only file systems cause crashes and prevent logging. #2856

Closed
TheSast opened this issue Jul 31, 2023 · 8 comments · Fixed by #2936
Closed
Labels
bug Something isn't working

Comments

@TheSast
Copy link

TheSast commented Jul 31, 2023

Describe the bug
Lazygit crashes after prompting for making a repo, before entering the TUI, if it cannot write to$XDG_CONFIG_HOME/lazygit/state.yml and it is not possible to log the issue using lazy --debug if $XDG_CONFIG_HOME/lazygit/development.log is also not able to be written to.
Both of those are inevitable to happen if the lazygit config folder is in a read-only file system.

To Reproduce
This is not the only way it can happen but it is how I specifically encountered the issue.
Steps to reproduce the an instance of the behavior:

  1. Install home-manager, back up your lazygit config folder and run home-manager init.
  2. Open $HOME/.config/home-manager/home.nix with a text editor.
  3. Add home.file.".config/lazygit/config.yml".text = ""; inside it's configuration.
  4. Run home-manager switch
  5. Run lazygit inside a git repository

Expected behavior
For program state files to respect $XDG_STATE_HOME as mentioned in #2794 with the expected default of $HOME/.local/state.
The XDG Base Directory Specification can be found here

Version info:
This issue happens in the current lazygit nix package for nix stable 23.05 (which is lazygit 0.38.2) and on a build from the current latest commit of this repository b92c294.
I have prepended the output of the lazygit --version command to the stderr output of lazygit of both builds.

Stderr Output:
commit=, build date=, build source=nix, version=0.38.2, os=linux, arch=amd64, git version=2.40.1

2023/07/31 21:16:03 An error occurred! Please create an issue at: https://github.com/jesseduffield/lazygit/issues

*fs.PathError open /home/u/.config/lazygit/state.yml: read-only file system
github.com/jesseduffield/lazygit/pkg/app/app.go:54 (0xab73dc)
github.com/jesseduffield/lazygit/pkg/app/entry_point.go:146 (0xab92da)
github.com/jesseduffield/lazygit/main.go:23 (0xabaebe)
runtime/internal/atomic/types.go:194 (0x439767)
runtime/asm_amd64.s:1598 (0x469b01)

commit=, build date=, build source=unknown, version=unversioned, os=linux, arch=amd64, git version=2.40.1

2023/07/31 21:31:33 An error occurred! Please create an issue at: https://github.com/jesseduffield/lazygit/issues

*fs.PathError open /home/u/.config/lazygit/state.yml: read-only file system
/home/u/go/pkg/mod/github.com/jesseduffield/[email protected]/pkg/app/app.go:55 (0xad7e9c)
	Run: newErr := errors.Wrap(err, 0)
/home/u/go/pkg/mod/github.com/jesseduffield/[email protected]/pkg/app/entry_point.go:151 (0xad9fa6)
	Start: Run(appConfig, common, appTypes.NewStartArgs(cliArgs.FilterPath, parsedGitArg, integrationTest))
/home/u/go/pkg/mod/github.com/jesseduffield/[email protected]/main.go:23 (0xadb85e)
	main: app.Start(ldFlagsBuildInfo, nil)
/nix/store/fp9k417pvah7d6ibbnz2x2llzframbk4-go-1.20.6/share/go/src/runtime/internal/atomic/types.go:194 (0x439767)
	(*Uint32).Load: return Load(&u.value)
/nix/store/fp9k417pvah7d6ibbnz2x2llzframbk4-go-1.20.6/share/go/src/runtime/asm_amd64.s:1598 (0x469b01)
	goexit: BYTE	$0x90	// NOP
@TheSast TheSast added the bug Something isn't working label Jul 31, 2023
@TheSast
Copy link
Author

TheSast commented Jul 31, 2023

Similar to #818 with same changes necessary for resolution.

@TheSast TheSast changed the title Read-only file system for config folder causes immediate crash. Unable to log due to same issue. (Caused by XDG BDS incompliance) Read-only config folder causes crash before TUI load Aug 3, 2023
@stefanhaller
Copy link
Collaborator

I can't reproduce this with a read-only config folder (i.e. chmod 555 "~/Library/Application Support/lazygit" on Mac). Lazygit starts normally, and displays the "Thanks for using lazygit" message every time, because it can't record that it did so in state.yml. It seems that you really need a read-only file system to trigger the crash, but I'm too lazy to set one up to try that.

There's code in app_config.go that checks for permission errors when writing the state file and silently ignores those. My guess is that the os.IsPermission function doesn't check for EROFS, so we might consider including that check ourselves at that point.

@stefanhaller
Copy link
Collaborator

@TheSast This should be fixed by #2936, could you test that PR please?

@TheSast
Copy link
Author

TheSast commented Jan 2, 2024

Tested that fork. It is a partial fix to what was mentioned in this issue.

  1. It does fix the error only for when state.yml is absent from the config folder, it yelds same error as before if present, I assume it tries to move it to the new location, this means the fix for that works as intended for home-manager users, since they probably wouldn't declare state.
  2. The log files are still written in the config folder making it impossible to check them for info.

But while testing I also encountered a similar issue for config.yml, lazygit will refuse to launch if it can't write config.yml.
Assuming it only tries to write to it if it's absent this won't be an issue for any home-manager users (like in my case or #818), since the folder is read-only only if it's declared, but one would have no reason declare it without the config file, but it may be possible that someone sets XDG_CONFIG_HOME to point to a read-only file system manually or through other means, which would make this behavior (and the state.yml migration behavior) an issue.

If it is possible, I'd suggest:

  1. (As you mentioned previously) specifically checking for read-only file systems (both for the config and state directories) and skipping any writing logic for those cases.
  2. Flag to specify where to write the log file (with - or no argument resulting in it going to stdout)

@TheSast TheSast changed the title Read-only config folder causes crash before TUI load Config and/or state files in read-only file systems cause crashes and prevent logging. Jan 2, 2024
@stefanhaller
Copy link
Collaborator

Thanks for testing!

  1. It does fix the error only for when state.yml is absent from the config folder, it yelds same error as before if present, I assume it tries to move it to the new location,

No, it writes to the file again in the same location where it found it. Which means that for existing users it will continue to save state.yml in the same folder as config.yml, forever. I don't think that's a problem, since state.yml is only created by lazygit, so if it exists in the config folder, that folder must be writeable.

We did consider moving files to the new locations, but decided against it in #2936.

  1. The log files are still written in the config folder making it impossible to check them for info.

Ah, I missed this one. Log files shouldn't be saved into the config folder. According to the XDG spec they should probably go to XDG_STATE_HOME. @horriblename, could you add that change to #2936? It looks like this will just be a one-line change to the LogPath() function at the end of app_config.go.

But while testing I also encountered a similar issue for config.yml, lazygit will refuse to launch if it can't write config.yml.

Again, only on a read-only file system. I can't reproduce with a folder that doesn't have write permissions.

But anyway, since this is a slightly unrelated problem, I think it should be fixed in a different PR (if at all). It doesn't seem like a serious issue to me.

To sum it up, with the additional change of writing log files to XDG_STATE_HOME we're good in my opinion.

@horriblename
Copy link
Contributor

horriblename commented Jan 5, 2024

According to the XDG spec they should probably go to XDG_STATE_HOME. @horriblename, could you add that change to #2936?

log files belong in XDG_CACHE_HOME I think. Logs aren't part of the app state after all.

I can add it in #2936 but would probably be better to split it off in another PR, what do you think?

@TheSast
Copy link
Author

TheSast commented Jan 5, 2024

Just for reference: https://specifications.freedesktop.org/basedir-spec/0.8/ar01s03.html

The $XDG_STATE_HOME contains state data that should persist between (application) restarts, but that is not important or portable enough to the user that it should be stored in $XDG_DATA_HOME. It may contain:

  • actions history (logs, history, recently used files, …)

@horriblename
Copy link
Contributor

seems reasonable to put it in XDG_STATE_HOME, will do that later in #2936

stefanhaller added a commit that referenced this issue Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants