Skip to content

Conversation

AndiDog
Copy link
Contributor

@AndiDog AndiDog commented Feb 3, 2023

Purpose

Given:

  • Syncthing home directory is owned by user OWNER and group GROUP
  • Syncthing runs as user NOTOWNER who is member of group GROUP

In this case, Syncthing cannot chmod the configuration file config.xml while saving it:

Saving config: chmod /[...]/config.xml: operation not permitted

That is because only the owner, not the group, can change permissions. Observed on Linux.

Since the config file is first atomically written, and then chmod attempted, the config file still gets written. But the Save function logs and returns this error and therefore also shows an error dialog in the GUI. To avoid running into this, my change only calls Chmod(...) if permissions differ [UPDATE] only fails after a Chmod(...) error if desired permissions really differ.

Testing

Ran Syncthing server before and after the change. The fix avoids any errors while saving the config file, given my scenario described above.

@imsodin
Copy link
Member

imsodin commented Feb 4, 2023

This adds another operation for the path where chmod is working. What about doing the check/stat only if chmod failed. And then if permissions are the same, ignore the chmod error.

@AndiDog
Copy link
Contributor Author

AndiDog commented Feb 4, 2023

Also very reasonable. Pushing that change – tested and working fine as well.

@AndiDog AndiDog force-pushed the chmod-only-if-needed branch from e713088 to 4d72458 Compare February 4, 2023 18:25
@AndiDog AndiDog changed the title Only try chmod (e.g. on config file) if permissions changed Only fail after chmod error if permissions differ (e.g. on config file) Feb 4, 2023
@AndiDog
Copy link
Contributor Author

AndiDog commented Feb 20, 2023

@imsodin Want to have another look if the current changes are what you desired?

@imsodin imsodin merged commit 2f88daf into syncthing:main Feb 20, 2023
calmh added a commit to imsodin/syncthing that referenced this pull request Mar 10, 2023
* main: (46 commits)
  build: Update dependencies
  lib/api: Expose `blocksHash` in file info (syncthing#8810)
  gui, man, authors: Update docs, translations, and contributors
  lib/discover: Don't leak relay-tokens to discovery (syncthing#8762)
  gui, man, authors: Update docs, translations, and contributors
  gui: Add Croatian (hr) translation template (syncthing#8801)
  build(deps): bump github.com/quic-go/quic-go from 0.32.0 to 0.33.0 (syncthing#8800)
  cmd/stupgrades: Cache should apply to HEAD as well as GET
  build: Add more GitHub Actions
  gui: Remove non-existent customicons.css file reference (fixes syncthing#8797) (syncthing#8798)
  Only fail after chmod error if permissions differ (e.g. on config file) (syncthing#8771)
  gui, man, authors: Update docs, translations, and contributors
  build: Use Go 1.19.6 for Windows
  build: Update dependencies
  gui, man, authors: Update docs, translations, and contributors
  gui: Remove duplicate Spanish (Spain) translation (fixes syncthing#8781) (syncthing#8782)
  gui: Add xattr filter editor (fixes syncthing#8660) (syncthing#8734)
  gui: Switch to Weblate for translations (syncthing#8777)
  all: Use new Go 1.19 atomic types (syncthing#8772)
  gui, man, authors: Update docs, translations, and contributors
  ...
calmh added a commit to tomasz1986/syncthing that referenced this pull request Mar 18, 2023
* main: (424 commits)
  gui, man, authors: Update docs, translations, and contributors
  lib/protocol: Cache expensive key operations (fixes syncthing#8599) (syncthing#8820)
  gui: Add Persian (fa) translation template (syncthing#8822)
  lib: Correctly handle encrypted trailer size (fixes syncthing#8556) (syncthing#8563)
  gui: Disable Restore Versions filters when no versioned files exist (fixes syncthing#5408) (syncthing#8539)
  build(deps): bump github.com/chmduquesne/rollinghash from 0.0.0-20180912150627-a60f8e7142b5 to 4.0.0+incompatible (syncthing#8804)
  build: Update dependencies (syncthing#8821)
  lib/api: Expose `blocksHash` in file info (syncthing#8810)
  gui, man, authors: Update docs, translations, and contributors
  lib/discover: Don't leak relay-tokens to discovery (syncthing#8762)
  gui, man, authors: Update docs, translations, and contributors
  gui: Add Croatian (hr) translation template (syncthing#8801)
  build(deps): bump github.com/quic-go/quic-go from 0.32.0 to 0.33.0 (syncthing#8800)
  cmd/stupgrades: Cache should apply to HEAD as well as GET
  build: Add more GitHub Actions
  gui: Remove non-existent customicons.css file reference (fixes syncthing#8797) (syncthing#8798)
  Only fail after chmod error if permissions differ (e.g. on config file) (syncthing#8771)
  gui, man, authors: Update docs, translations, and contributors
  build: Use Go 1.19.6 for Windows
  build: Update dependencies
  ...
@AndiDog AndiDog deleted the chmod-only-if-needed branch January 28, 2024 12:20
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jul 26, 2024
@syncthing syncthing locked and limited conversation to collaborators Jul 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants