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

Improve deletion of the Sandboxie folder #3044

Merged
merged 1 commit into from Jun 17, 2023
Merged

Conversation

isaak654
Copy link
Collaborator

@isaak654 isaak654 commented Jun 16, 2023

The uninstaller of Sandboxie Classic tends to leave the Sandboxie installation folder as empty in some cases, so I would suggest to further improve the deletion.

@isaak654 isaak654 added the Sbie installer Sandboxie installer label Jun 16, 2023
@isaak654
Copy link
Collaborator Author

@sredna Any feedback or input would be greatly appreciated.

@isaak654 isaak654 added the Confirmation pending Further confirmation is requested label Jun 16, 2023
@sredna
Copy link
Contributor

sredna commented Jun 16, 2023

RMDir (without /r) only deletes a directory if it is empty, you don't need a function. If it failed to delete then maybe something has a open handle, maybe call RMDir and Sleep in a loop while IfErrors is true after RMDir. Give up after 5 seconds.

Process Monitor will tell you why the delete fails.

DeleteDirIfEmpty is also broken, the returned order of . and .. is undefined.

@isaak654 isaak654 marked this pull request as draft June 16, 2023 19:54
@isaak654 isaak654 changed the title Delete installation directory only if empty Improve deletion of the Sandboxie folder Jun 17, 2023
@isaak654
Copy link
Collaborator Author

Process Monitor will tell you why the delete fails.

I've just tried, but I'm unable to reproduce this issue consistently. However, I would still like to provide a minimal fix that doesn't break anything.

If it failed to delete then maybe something has a open handle, maybe call RMDir and Sleep in a loop while IfErrors is true after RMDir. Give up after 5 seconds.

Please check my refreshed commit.

Also, I would like to know if setting a SetOutPath $TEMP before the first RMDir "$INSTDIR" could further help as in this example.

@sredna
Copy link
Contributor

sredna commented Jun 17, 2023

Please check my refreshed commit

You need a clearerrors before the first of the two rmdir's.

Also, I would like to know if setting a SetOutPath $TEMP before the first RMDir "$INSTDIR" could further help

It will not hurt anything. It is used because it calls SetCurrentDirectory and the current directory holds a handle. If the current directory was the problem then it should happen every time.

Your code is a bit strange with everything in functions and no uninstaller section?

@isaak654
Copy link
Collaborator Author

isaak654 commented Jun 17, 2023

Thanks for your report, a new commit was pushed.

Your code is a bit strange with everything in functions and no uninstaller section?

Frankly I'm not sure what was the intention of the original developer, basically the code now belongs to the Sandboxie community: https://news.sophos.com/en-us/2020/04/09/sandboxie-is-now-an-open-source-tool/

What we're missing

I think what we really miss at this time is the compatibility support, especially with UWP apps, and at least an independent security audit provided by a security research team... and yet, in spite of everything, over 20 security issues were fixed internally since the open-source release, but it's a bit unfair that other open-source software like Veracrypt receive more public attention in this regard than Sandboxie.

@isaak654 isaak654 removed the Confirmation pending Further confirmation is requested label Jun 17, 2023
@isaak654 isaak654 marked this pull request as ready for review June 17, 2023 21:41
@isaak654 isaak654 merged commit 3770420 into master Jun 17, 2023
1 check passed
@isaak654 isaak654 deleted the Classic_uninstall branch June 17, 2023 21:45
isaak654 added a commit that referenced this pull request Jun 17, 2023
isaak654 added a commit that referenced this pull request Jun 21, 2023
Follow-up of #3044
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sbie installer Sandboxie installer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants