lib: Don't add enc. trailer to info size (fixes #8556) #8563
Conversation
|
@imsodin Any chance we can get this into the upcoming release? |
|
Added a heuristic for the schema migration to detect affected files. Please re-review those bits. |
* main: (36 commits) lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655) lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650) gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984) gui, man, authors: Update docs, translations, and contributors build: Add GitHub actions build for Windows (syncthing#8627) gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644) gui: Adjust connection type icon size scaling and alignment (syncthing#8645) docker: Use healthcheck endpoint (syncthing#8640) lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631) gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530) gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571) gui: Always show Out of Sync Items for remote devices (syncthing#8632) lib/fs: Let xattr test avoid non-test attributes (fixes syncthing#8601) (syncthing#8628) build: Add GitHub actions build for Windows gui, man, authors: Update docs, translations, and contributors gui: Display folder and device count number (syncthing#8615) gui, man, authors: Update docs, translations, and contributors lib/model, lib/protocol: Fix file comparisons (fixes syncthing#8594) (syncthing#8603) lib/scanner: More sensible debug output (syncthing#8596) gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597) ...
calmh
left a comment
There was a problem hiding this comment.
There are some points of confusion, I'll hack it a little and see if I can make it work.
|
So, again, It's Not That Easy(tm). The scanner is fine now I think, that part works for me, though it is a bit slower than before because we actually need to open the file and read a few bytes to figure out it's pre-trailer size. Another option here might be to just not do that and instead ignore size when looking at receive-encrypted files... The migration is trickier because we don't easily have the size to subtract. It's not the I'm tempted to skip the migration and let revert-local-changes sort it out, either by triggering that automatically on upgrade or just noting that it needs to happen to clear out the state... |
|
Just a quick thought without having looked at the details... Could we add some kind of version field to the trailer? If it doesn't match what we use (from now on), the info is nuked and recalculated? Kind of an on-the-fly migration. |
|
We should probably only write The in the migration we'd need to be even smarter, rewriting the files and such. Really tempted to just drop it all and redo then, but in this case I do think we should combine it with #7987 and shortening the path segments - meaning delay the fix. Sure it's labeled beta, but doing a full drop twice seems quite mean. |
|
Well, the "it's not that easy" referred to the current state where it's quite difficult to do the migration. But it's also not that easy that we can just write the encrypted part, though it would have been convenient, because the key to the encryption of that bundle and the file itself is based on the file name, which we get from decrypting the name in the "outer" fileinfo using the folder key... We could add a local attribute to the fileinfo to store the file-on-disk-with-trailer for comparison purposes, or we could just not try to detect random size changes to encrypted files in the scanner... |
|
Ok, next try. Motivation: I don't want to open files while scanning, and I still want to detect changed files. Also have a transition such that the existing setup stays usable, because it was simple-ish to do with the solution I came up with for the previous motivations. And fixing the bug of course. Approach: Add a new field to the file-info, for the trailer size. Then still add the trailer size to the file-size to get an accurate total local size of a folder. Use the trailer size field in the index sender to send the correct (no trailer) size to remotes (fixes this bug). I tested it locally and no local changes happened on the transition. |
* 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 ...
* main: 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)
* main: 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)
* main: 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)
* main: 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)
* 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 ...
…ng#8556) In the original fix in syncthing#8563 I simply forgot this. Which meant syncthing#8556 wasn't actually fixed, as the trialer size would have been 0 (default), and thus we would have still sent the inflated size to encrypted peers.
Fixes a regression due to PR syncthing#8563, while arguable the bug was actually introduced in a much older PR syncthing#7155, but didn't have any bad effects so far: We account for the encryption trailer in the db updater routine, calculating the file-info size there. However there's no guarantee that the file-info at this point is still the exact same as when it was written. It was before, but isn't anymore since introducing the new EncryptedTrailerSize field. Fix: Adjust the size in the info at the same place where the trailer is written, i.e. we definitely have the actual size on disk.
lib/model: Fix file size inconsisency due to enc. trailer Fixes a regression due to PR #8563, while arguable the bug was actually introduced in a much older PR #7155, but didn't have any bad effects so far: We account for the encryption trailer in the db updater routine, calculating the file-info size there. However there's no guarantee that the file-info at this point is still the exact same as when it was written. It was before, but isn't anymore since introducing the new EncryptedTrailerSize field. Fix: Adjust the size in the info at the same place where the trailer is written, i.e. we definitely have the actual size on disk.
…ng#8556) In the original fix in syncthing#8563 I simply forgot this. Which meant syncthing#8556 wasn't actually fixed, as the trialer size would have been 0 (default), and thus we would have still sent the inflated size to encrypted peers.
* main: (28 commits) build: Update dependencies (syncthing#8869) lib/model: Improve path generation for auto accepted folders (fixes syncthing#8859) (syncthing#8860) docs: fix typo (syncthing#8857) gui, man, authors: Update docs, translations, and contributors lib/syncthing: Handle successful global migration (fixes syncthing#8851) (syncthing#8852) gui, man, authors: Update docs, translations, and contributors lib/model: Set enc. trailer size on pull (ref syncthing#8563, syncthing#8556) (syncthing#8839) lib/model: Fix file size inconsistency due to enc. trailer (syncthing#8840) gui, man, authors: Update docs, translations, and contributors cmd/stdiscorv: Fix database test (fixes syncthing#8828) lib/ur: Fix custom releases URL comparison gui: Remove untranslated strings from JSON files (syncthing#8836) all: Fix typos found by codespell (syncthing#8833) gui: Hide download progress legend when download progress is disabled gui, man, authors: Update docs, translations, and contributors lib/protocol: Handle encrypted requests without encrypted hash (fixes syncthing#8277) (syncthing#8827) build(deps): bump github.com/hashicorp/golang-lru/v2 from 2.0.1 to 2.0.2 (syncthing#8826) lib/config: Allow sub-second watcher delay (fixes syncthing#7859) (syncthing#7864) gui, man, authors: Update docs, translations, and contributors lib/protocol: Cache expensive key operations (fixes syncthing#8599) (syncthing#8820) ...
Untested so far.