On Windows, sentry__process_old_runs can remove the wrong folders

[mcdurdin]

The call to SHFileOperationW in sentry__path_remove_all when cleaning up can go disastrously wrong. Per documentation, it must never be called with relative paths because it is not thread safe. Second, I have seen old comments online about it not handling forward slashes.

How disastrous is disastrous? I was running my test app from a folder c:\projects\keyman\app\windows\bin\engine and it deleted everything in c:\projects up until it failed with a file in use: sentry.dll itself. That was many thousands of folders and hundreds of thousands of files -- I was wondering why it was so slow to start. I am not sure how much else I've lost from that Projects folder; most everything is on GitHub but I don't remember the 60-70 projects there (there are 5 left). Of course, when this happened I had no idea what had caused it. To trace the cause I had to experience this several times.

🤕

I had not specified database_path in my test app, relying on the default of "./.sentry-native".

Thoughts about fixing this properly:

1. Never use SHFileOperationW (or any shell file operation style functions). It's not thread safe and introduces GUI dependencies, unwanted for console apps or services:
   • oldnewthing ref 1, Raymond Chen says "inadvisable".
   • oldnewthing ref 2, he extends that to "flat-out wrong (with caveat)".
   • oldnewthing ref 3, he says still wrong but if you are going to ignore his advice, at least use a sterilized needle.
2. The practice of using a relative path, anywhere, is not robust. Relative paths must be converted to absolute paths on first use to avoid further disaster when the app changes directory during run.
3. On Windows, you should really be using a local appdata folder (e.g. %LOCALAPPDATA% or, if you don't mind depending on shell32.dll, SHGetKnownFolderPath(FOLDERID_LocalAppData)) as the default for any writable data. In normal use, most apps installed to Program Files will end up with a non-writable path as their working directory. Don't forget to expand the environment string.
4. The default database path is "./.sentry-native". sentry_path_t should be converting all forward slashes to backslashes on Windows as forward slash is inconsistently supported.

[Swatinem]

I’m really sorry this has happened to you :-(

The disastrously bug in the function was that it basically read uninitialized memory, which will be fixed here:

https://github.com/getsentry/sentry-native/pull/209/files#diff-67f64c604f4320bd02f6ff8ca61f441bR322

I guess I was lucky that it didn’t randomly delete my whole FS basically, and it seems you are the first to be hit by this.

[mcdurdin]

Thanks mate! All good here because I didn’t lose anything significant (just a few years of my life 😂).

The uninitialised data makes more sense of the circumstances to me. I’d still urge you not to use SHFileOperation though!

[Swatinem]

I just looked at this, and even the API docs say here: https://docs.microsoft.com/de-de/windows/win32/api/fileapi/nf-fileapi-removedirectoryw#remarks

To recursively delete the files in a directory, use the SHFileOperation function.

What would you rather recommend?

[mcdurdin]

I would probably use a FindFirstFile / FindNextFile loop. More code but doesn’t introduce the shell32 can of worms.

[Swatinem]

Interestingly, we also use the SHCreateDirectoryExW function from shell32.
Both of these are basically the equivalent of mkdir -p and rm -rf.

The unix version of this function actually does a dir walk with a recursion, so I guess its not a bad idea to use that on windows as well. Similarly for mkdir -p.

And yes, this issue here kind of reminds me of MrMEEE/bumblebee-Old-and-abbandoned#123 🤦‍♂