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

Comparing 2 snapshots can fill your disk if it includes a symlink to e.g. / (doesn't copy symlink as symlink) #1902

Closed
sevens opened this issue Oct 12, 2024 · 7 comments · Fixed by #1937
Labels
Bug Discussion decision or consensus needed Low relevant, but not urgent

Comments

@sevens
Copy link
Contributor

sevens commented Oct 12, 2024

First of all thanks for the software, have been using it quite a bit through the years!

I was trying to compare 2 snapshots (one of <30GB with one of a few 100MB) and suddenly noticed my system acting weird due to what turned out to be my disk being full, even though nearly 1TB of 1.8TB was free beforehand...

Long story short, it turned out that symlinks in the snapshots were having their targets copied as a regular dir (instead of being copied as symlinks themselves) to the temp location BiT uses for comparing. The symlink's target included another symlink somewhere within, this one pointing to / (Proton install from Steam which has a z: drive/symlink pointing to /). That resulted in an infinite dir structure being copied: / includes ~/.steam/root/steamapps/compatdata/ID/pfx/dosdevices/z: which points to / which includes ~/.steam/root/steamapps/compatdata/ID/pfx/dosdevices/z: which points to ...

After digging into the source a bit it seems that BiT creates a copy of the snapshots its comparing (

path1 = self.parent.tmpCopy(path1, sid1)
and line 372). This uses Python's shutil.copytree (
shutil.copytree(full_path, tmp_file)
) which by default (and in the form BiT calls it) doesn't copy symlinks as symlinks but copies their targets:

If symlinks is true, symbolic links in the source tree are represented as symbolic links in the new tree and the metadata of the original links will be copied as far as the platform allows; if false or omitted, the contents and metadata of the linked files are copied to the new tree. [Default is False]
-- https://docs.python.org/3/library/shutil.html#shutil.copytree

presumably causing the issue.

Easy reproduction of the base issue:

  • Create 2 snapshots, at least one of them containing a symlink
  • Compare the 2 snapshots
  • Check the temp dir of the snapshots and notice there is no symlink but the target is included instead
    • E.g.:

      $ ls -l ~/backup_source/
      total 0
      lrwxrwxrwx 1 USER users 3 10-12 15:16 bar -> foo
      -rw-r--r-- 1 USER users 0 10-12 15:48 baz
      -rw-r--r-- 1 USER users 8 10-12 15:48 foo
      
      $  ls -l ~/backup_target/backintime/HOST/USER/PROFILE_NAME/last_snapshot/backup/home/USER/backup_source/
      total 4
      lrwxrwxrwx 2 USER users 3 10-12 15:16 bar -> foo
      -rw-r--r-- 1 USER users 0 10-12 15:48 baz
      -rw-r--r-- 2 USER users 8 10-12 15:48 foo
      
      $ ls -l /tmp/tmp*/backup_source/
      /tmp/tmpebgjzrbp_20241012-154851-538/backup_source/:
      total 8
      -rw-r--r-- 1 USER users 8 10-12 15:48 bar
      -rw-r--r-- 1 USER users 8 10-12 15:48 foo
      
      /tmp/tmpfvmhdn16_20241012-154902-833/backup_source/:
      total 8
      -rw-r--r-- 1 USER users 8 10-12 15:48 bar
      -rw-r--r-- 1 USER users 0 10-12 15:48 baz
      -rw-r--r-- 1 USER users 8 10-12 15:48 foo

To probably reproduce the exact situation (NOT RECOMMENDED!), written from memory as I don't want to try it again to verify:

  • Have 2 snapshots that somewhere (indirectly) include a symlink to / (e.g. Steam with a Proton install or a regular Wine environment)
    • NOTE: my snapshots didn't have a direct symlink to /. The snapshot included my ~/.steam which has a root -> /home/USER/.local/share/Steam symlink but ~/.local/share/Steam itself is excluded from the snapshots (wouldn't matter here as the symlink uses an absolute path so it points to the live data, not the snapshot's).
      • And /home/USER/.local/share/Steam includes content that has symlinks pointing to /.
  • Compare the 2 entire snapshots
    • This will fill up /tmp.
  • If /tmp if not a tmpfs that disk should now be full
  • In case /tmp is a tmpfs (it is in my case), now fill up an actual disk:
    • Now that /tmp is full, kill Back in Time
    • Start a new Back in Time and compare the 2 snapshots again, it will now use /var/tmp (presumably because /tmp is now full so Python will pick something else for its temp location)
    • Let it fill /var/tmp and enjoy the disk being full
  • Once satisfied, manually cleanup the temp BiT content in /tmp and /var/tmp to get a sane system again
`backintime --diagnostics`
{
    "backintime": {
        "name": "Back In Time",
        "version": "1.4.3",
        "latest-config-version": 6,
        "local-config-file": "/home/UsernameReplaced/.config/backintime/config",
        "local-config-file-found": true,
        "global-config-file": "/etc/backintime/config",
        "global-config-file-found": false,
        "started-from": "/usr/share/backintime/common",
        "running-as-root": false,
        "user-callback": "/home/UsernameReplaced/.config/backintime/user-callback",
        "keyring-supported": true
    },
    "host-setup": {
        "platform": "Linux-6.11.2-1-default-x86_64-with-glibc2.40",
        "system": "Linux #1 SMP PREEMPT_DYNAMIC Fri Oct  4 17:37:58 UTC 2024 (38c846e)",
        "OS": "openSUSE Tumbleweed",
        "display-system": "tty",
        "locale": "en_US, UTF-8",
        "PATH": "/home/UsernameReplaced/.local/bin:/home/UsernameReplaced/.local/bin:/home/UsernameReplaced/.local/bin:/usr/local/bin:/usr/bin:/bin",
        "RSYNC_OLD_ARGS": "(not set)",
        "RSYNC_PROTECT_ARGS": "(not set)"
    },
    "python-setup": {
        "python": "3.11.10 main Sep 09 2024 17:03:08 CPython GCC",
        "python-executable": "/usr/bin/python3",
        "python-executable-symlink": true,
        "python-executable-resolved": "/usr/bin/python3.11",
        "sys.path": [
            "/usr/share/backintime/qt/plugins",
            "/usr/share/backintime/common/plugins",
            "/usr/share/backintime/plugins",
            "/usr/share/backintime/common",
            "/usr/lib64/python311.zip",
            "/usr/lib64/python3.11",
            "/usr/lib64/python3.11/lib-dynload",
            "/usr/lib64/python3.11/site-packages",
            "/usr/lib64/python3.11/_import_failed",
            "/usr/lib/python3.11/site-packages"
        ],
        "qt": {
            "Version": "PyQt 5.15.10 / Qt 5.15.15",
            "Theme": "hicolor",
            "Theme Search Paths": [
                "/usr/share/icons",
                ":/icons"
            ],
            "Fallback Theme": "hicolor",
            "Fallback Search Paths": [
                "/usr/share/pixmaps"
            ]
        }
    },
    "external-programs": {
        "rsync": {
            "version": "3.3.0",
            "protocol": "31.0",
            "capabilities": {
                "file_bits": 64,
                "inum_bits": 64,
                "timestamp_bits": 64,
                "long_int_bits": 64,
                "socketpairs": true,
                "symlinks": true,
                "symtimes": true,
                "hardlinks": true,
                "hardlink_specials": true,
                "hardlink_symlinks": true,
                "IPv6": true,
                "atimes": true,
                "batchfiles": true,
                "inplace": true,
                "append": true,
                "ACLs": true,
                "xattrs": true,
                "secluded_args": "optional",
                "iconv": true,
                "prealloc": true,
                "stop_at": true,
                "crtimes": false
            },
            "optimizations": {
                "SIMD_roll": true,
                "asm_roll": false,
                "openssl_crypto": true,
                "asm_MD5": false
            },
            "checksum_list": [
                "xxh128",
                "xxh3",
                "xxh64",
                "md5",
                "md4",
                "sha1",
                "none"
            ],
            "compress_list": [
                "zstd",
                "lz4",
                "zlibx",
                "zlib",
                "none"
            ],
            "daemon_auth_list": [
                "sha512",
                "sha256",
                "sha1",
                "md5",
                "md4"
            ]
        },
        "ssh": "OpenSSH_9.9p1, OpenSSL 3.1.4 24 Oct 2023",
        "sshfs": "3.7.4a",
        "encfs": "1.9.5",
        "shell": "/bin/bash",
        "shell-version": "GNU bash, version 5.2.37(1)-release (x86_64-suse-linux)"
    }
}

Original problem on openSUSE Tumbleweed (installed from the main repo, diagnostics included above), basic issue reproduced on Slackware (Back in Time 1.4.3, Python 3.11.10, installed via slackbuilds.org).

@buhtz
Copy link
Member

buhtz commented Oct 12, 2024

Hello Peter,

Thank you for taking the time to report the bug and providing the
details. I appreciate your feedback.

I never understood why BIT is coping the snapshots before comparing it. I am not deep enough into this feature of BIT so I can not decide yet. But I am assuming that comparing (diff snapshot) can be achieved without copy the snapshot. IN this case your issue would be obsolete.

If you have any more details to share, feel free to reach out.

Not sure when we'll find the time to work on it. Please see the projects background information to get an idea about our workflow and priorities:

Best regards,
Christian

@buhtz buhtz added Bug Low relevant, but not urgent Discussion decision or consensus needed labels Oct 12, 2024
@buhtz buhtz added this to the 3rd release from now milestone Oct 12, 2024
@sevens
Copy link
Contributor Author

sevens commented Oct 12, 2024

Hello Christian,

Thank you for taking the time to report the bug and providing the details. I appreciate your feedback.

You're welcome!

I never understood why BIT is coping the snapshots before comparing it. I am not deep enough into this feature of BIT so I can not decide yet.

According to

# prevent backup data from being accidentally overwritten
# by create a temporary local copy and only open that one
it's to avoid accidentally overriding, which makes sense to me: the diff (if copying of the snapshots ever finishes, which it normally should ;P ) gets opened in a merge tool and if one accidentally clicks on e.g. "apply diff" and saves it the actual snapshot would get modified if no copy was used, which is probably not what you want/expect :)

But I am assuming that comparing (diff snapshot) can be achieved without copy the snapshot. IN this case your issue would be obsolete.

It would avoid this issue though it'd create the risk of modifying the (or both?) snapshot(s)? Though from reading the linked Python docs I'd expect it to also be fixed by simply adding symlinks=True to the shutil.copytree() call so it creates a copy of what the source actually is (which to me seems better than the current situation anyway, at least as long as the current code is in use).

If you have any more details to share, feel free to reach out.

Not sure when we'll find the time to work on it. Please see the projects background information to get an idea about our workflow and priorities:

No worries. I've been reading a bit up on it the last few weeks already.

Best regards,
Christian

Regards,
Peter.

@buhtz
Copy link
Member

buhtz commented Oct 13, 2024

to avoid accidentally overriding, which makes sense to me: the diff (if copying of the snapshots ever finishes, which it normally should ;P ) gets opened in a merge tool and if one accidentally clicks on e.g. "apply diff" and saves it the actual snapshot would get modified if no copy was used, which is probably not what you want/expect :)

Mhm... I see. But if it is a 10 GB snapshot a lot things need to get copied. OK, OK, but I get it.

It would avoid this issue though it'd create the risk of modifying the (or both?) snapshot(s)? Though from reading the linked Python docs I'd expect it to also be fixed by simply adding symlinks=True to the shutil.copytree() call so it creates a copy of what the source actually is (which to me seems better than the current situation anyway, at least as long as the current code is in use).

Sounds easy. Would you like to contribute a pull request?
I looked into the unit tests but couldn't find a test covering the "compare snapshot behavior". Maybe you find one? Not sure if it makes sense to test it.

Regards, Christian

sevens added a commit to sevens/backintime that referenced this issue Nov 21, 2024
When comparing snapshots, copy symlinks in the snapshots as symlinks, not as
their target.  The old behavior made the comparison not fully compare the actual
snapshot data and, even worse, could fill up your entire disk (when having
circular symlinks, e.g. a symlink to `/`, which copies `/` and eventually
copies the original symlink which points to `/` which ...).

Additionally fixes a crash when the compared snapshot(s) contain a symlink
pointing to a nonexistent target.

Fixes bit-team#1902.
sevens added a commit to sevens/backintime that referenced this issue Nov 21, 2024
When comparing snapshots, copy symlinks in the snapshots as symlinks, not as
their target.  The old behavior made the comparison not fully compare the actual
snapshot data and, even worse, could fill up your entire disk (when having
circular symlinks, e.g. a symlink to `/`, which copies `/` and eventually
copies the original symlink which points to `/` which ...).

Additionally fixes a crash when the compared snapshot(s) contain a symlink
pointing to a nonexistent target.

Fixes bit-team#1902.
sevens added a commit to sevens/backintime that referenced this issue Nov 21, 2024
When comparing snapshots, copy symlinks in the snapshots as symlinks, not as
their target.  The old behavior made the comparison not fully compare the actual
snapshot data and, even worse, could fill up your entire disk (when having
circular symlinks, e.g. a symlink to `/`, which copies `/` and eventually
copies the original symlink which points to `/` which ...).

Additionally fixes a crash when the compared snapshot(s) contain a symlink
pointing to a nonexistent target.

Fixes bit-team#1902.
sevens added a commit to sevens/backintime that referenced this issue Nov 21, 2024
When comparing snapshots, copy symlinks in the snapshots as symlinks, not as
their target.  The old behavior made the comparison not fully compare the actual
snapshot data and, even worse, could fill up your entire disk (when having
circular symlinks, e.g. a symlink to `/`, which copies `/` and eventually
copies the original symlink which points to `/` which ...).

Additionally fixes a crash when the compared snapshot(s) contain a symlink
pointing to a nonexistent target.

Fixes bit-team#1902.
@sevens
Copy link
Contributor Author

sevens commented Nov 21, 2024

to avoid accidentally overriding, which makes sense to me: the diff (if copying of the snapshots ever finishes, which it normally should ;P ) gets opened in a merge tool and if one accidentally clicks on e.g. "apply diff" and saves it the actual snapshot would get modified if no copy was used, which is probably not what you want/expect :)

Mhm... I see. But if it is a 10 GB snapshot a lot things need to get copied. OK, OK, but I get it.

Yeah, there's definitely downsides to it as well. Also as a user it wasn't clear to me a copy was going to be made in the first place. Maybe a longer term solution could be to let the user decide and choose between speed or risk? (though dialogs have their own downsides)

Also just realized that these temp copies will have no deduplication on them. So a bad case when comparing full snapshots (assuming that when comparing only a part of a snapshot, only the relevant part gets copied from both snapshots) you might need twice the space of the original data available in a temp location just to host the 2 copies

It would avoid this issue though it'd create the risk of modifying the (or both?) snapshot(s)? Though from reading the linked Python docs I'd expect it to also be fixed by simply adding symlinks=True to the shutil.copytree() call so it creates a copy of what the source actually is (which to me seems better than the current situation anyway, at least as long as the current code is in use).

Sounds easy. Would you like to contribute a pull request? I looked into the unit tests but couldn't find a test covering the "compare snapshot behavior". Maybe you find one? Not sure if it makes sense to test it.

Sure (had some other higher priority stuff to do first).

I searched for unittests too but couldn't find anything. Searched for 'compare', case insensitive, in both common/test/ and qt/test/ and couldn't find anything relevant. In fact, qt/test (the relevant code is in qt/) only seems to contain some lint tests, otherwise I'd have tried to at least add a unittest for app.MainWindow.tmpCopy() itself

Additionally, upon reading the Python copytree docs a bit more, I found out that when comparing a snapshot containing a symlink to a nonexistent target, Back in Time GUI will crash entirely (see the pull request for a detailed example). This is also fixed by using symlinks=True.

@sevens
Copy link
Contributor Author

sevens commented Nov 21, 2024

Also, while on the subject, the names of the temp dirs are very generic and don't really indicate they're from Back in Time (the snapshot name is in there but as that's just a bunch of datetime stamps and a random(?) suffix it's hard to tell it's from BiT), e.g.: /tmp/tmpvvv2e1nz_20241121-112648-601. This makes it harder to know if it's safe to remove them manually (BiT only seems to clean them up if it exits normally, which at least it won't when the process hangs due to filling the disk and needs to be killed).

Would it make sense to add an explicit prefix to the temp dir as well? E.g. backintime-snapshot_compare_ or something (resulting in temp names like /tmp/backintime-snapshot_compare_q9w_sfxn_SNAPSHOT_ID)? I don't mind creating a pull request for this too (it's just another argument to the TemporaryDirectory()) but thought I'd better check first. And, if so, is there a preference/recommendation on the prefix?

@buhtz
Copy link
Member

buhtz commented Nov 21, 2024

Thank you very much, I will need a bit time to have a deeper look into it.
I agree about the prefix for a temp dir. But I suggest to realize this in a separate PR.

sevens added a commit to sevens/backintime that referenced this issue Nov 21, 2024
Back in Time will sometimes create a temp dir, to copy (parts of) a snapshot.
These have a generic name, in the form of `tmpXXXXXXXX_SNAPSHOTID`, where
`XXXXXXXX` are random characters and `SNAPSHOTID` is the snapshot id (e.g.
`20241121-112648-601`).  Only the latter gives a hint that these dirs are from
Back in Time but it's hard to tell as they're just some numbers.

This makes it tricky for the user to know if it's safe to delete these dirs if
they're left around (Back in Time will remove them if it exits cleanly but
that's not always possible, e.g. might hang if disk space gets full, like
in bit-team#1902 or when comparing too large snapshots).  Added an explicit
`backintime_` prefix to the temp dir to make their origin more clear.
sevens added a commit to sevens/backintime that referenced this issue Nov 21, 2024
Back in Time will sometimes create a temp dir, to copy (parts of) a snapshot.
These have a generic name, in the form of `tmpXXXXXXXX_SNAPSHOTID`, where
`XXXXXXXX` are random characters and `SNAPSHOTID` is the snapshot id (e.g.
`20241121-112648-601`).  Only the latter gives a hint that these dirs are from
Back in Time but it's hard to tell as they're just some numbers.

This makes it tricky for the user to know if it's safe to delete these dirs if
they're left around (Back in Time will remove them if it exits cleanly but
that's not always possible, e.g. might hang if disk space gets full, like
in bit-team#1902 or when comparing too large snapshots).  Added an explicit
`backintime_` prefix to the temp dir to make their origin more clear.
@sevens
Copy link
Contributor Author

sevens commented Nov 21, 2024

Thank you very much, I will need a bit time to have a deeper look into it.

You're welcome, and no problem.

I agree about the prefix for a temp dir. But I suggest to realize this in a separate PR.

Yeah, was already planning on using a separate PR (but didn't mention it), I split up the 2 PRs for CONTRIBUTING.md for similar reasons :) Created PR: #1940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Discussion decision or consensus needed Low relevant, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants