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

invalid temp_folder_path deletes entire folder tree where epub file exists on open #285

Closed
UberBen opened this Issue Mar 13, 2017 · 24 comments

Comments

Projects
None yet
4 participants
@UberBen

UberBen commented Mar 13, 2017

If 'temp_folder_path' points to an invalid folder, opening an epub file by double-clicking results in the entire directory structure to be deleted where the epub resides, then gives the following error message:

Error info: /OEBPS/content.opf: The system cannot find the path specified.
Sigil version: 0.9.7
Runtime Qt: 5.6.1
Compiled Qt: 5.6.1
Platform: Windows SysInfo ID 192

It deleted my entire Downloads folder and all subfolders when this happens. fixing temp_folder_path resolved the issue.

@UberBen UberBen changed the title from invalid temp_folder_path deletes entire folder tree where epub files exists on open to invalid temp_folder_path deletes entire folder tree where epub file exists on open Mar 13, 2017

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

It looks like if temp_folder_path is invalid, it sets the temporary folder to the current directory. It then wipes the current directory when Sigil exits on error to clean up the new temp folder.

UberBen commented Mar 13, 2017

It looks like if temp_folder_path is invalid, it sets the temporary folder to the current directory. It then wipes the current directory when Sigil exits on error to clean up the new temp folder.

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor
Contributor

kevinhendricks commented Mar 13, 2017

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor

Also, it should only be creating a Sigil folder with a uuid name in that location and only should be deleting that folder not everything in the temp folder, so something else is going on at the Windows level.

Contributor

kevinhendricks commented Mar 13, 2017

Also, it should only be creating a Sigil folder with a uuid name in that location and only should be deleting that folder not everything in the temp folder, so something else is going on at the Windows level.

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

This is a Windows 10 machine. I haven't changes any ini settings. I only use Sigil on this machine to add covers to my epubs converted from fanfictiondownloader. the temp_folder_path was set to "C:\Users\bwinter\AppData\Local\Temp\4" and my current temp folder is actually "C:\Users\bwinter\AppData\Local\Temp\1" so some other program may be moving my temp folder around and confusing Sigil.

UberBen commented Mar 13, 2017

This is a Windows 10 machine. I haven't changes any ini settings. I only use Sigil on this machine to add covers to my epubs converted from fanfictiondownloader. the temp_folder_path was set to "C:\Users\bwinter\AppData\Local\Temp\4" and my current temp folder is actually "C:\Users\bwinter\AppData\Local\Temp\1" so some other program may be moving my temp folder around and confusing Sigil.

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

If you open Preferences, then click OK without changing anything, it adds the temp_folder_path entry with the hard-coded temp folder.

UberBen commented Mar 13, 2017

If you open Preferences, then click OK without changing anything, it adds the temp_folder_path entry with the hard-coded temp folder.

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor

Just tested with my own Mac machine and set the temp folder to /Users/kbhend/Desktop/junk/mytemp and copied a bunch of other files into that folder. Set that folder using Sigil Preferences. Then watched it create a Sigil-FaeKyd random directory name. Upon exiting Sigil, only that Sigil-Faekyd directory was removed none of the other files inside it were touched. No recursive delete was up up the path at all. None of the other junk files I put in that directory as potential guinea pigs were touched in any way.

That is how it was planned to work.

It seems windows Qt make secure temporary directory call is not doing its thing. No matter any existing path chosen (such as your Downloads folder), it should be creating a temporary Sigil specific folder in that directory and only deleting its own Sigil temporary folder.

I have no idea how Qt's call to securely create temporary Sigil specific folder is failing no matter where your temp is located.

Perhaps this is a Qt bug. I will do some digging. I can also add a check that the temp folder path set in the ini file exists and if not abort Sigil but it should not be needed.

Contributor

kevinhendricks commented Mar 13, 2017

Just tested with my own Mac machine and set the temp folder to /Users/kbhend/Desktop/junk/mytemp and copied a bunch of other files into that folder. Set that folder using Sigil Preferences. Then watched it create a Sigil-FaeKyd random directory name. Upon exiting Sigil, only that Sigil-Faekyd directory was removed none of the other files inside it were touched. No recursive delete was up up the path at all. None of the other junk files I put in that directory as potential guinea pigs were touched in any way.

That is how it was planned to work.

It seems windows Qt make secure temporary directory call is not doing its thing. No matter any existing path chosen (such as your Downloads folder), it should be creating a temporary Sigil specific folder in that directory and only deleting its own Sigil temporary folder.

I have no idea how Qt's call to securely create temporary Sigil specific folder is failing no matter where your temp is located.

Perhaps this is a Qt bug. I will do some digging. I can also add a check that the temp folder path set in the ini file exists and if not abort Sigil but it should not be needed.

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

It looks like Windows Updates moved my Windows temp folder yesterday. Sigil worked fine on Friday.

I can recreate the problem by installing Sigil from scratch, open Preferences then click OK without changing anything. Then point my Windows temp folder to another folder. I then double-click on an epub file and it will instantly delete everything in the folder that the epub file was in, including all subfolders.

UberBen commented Mar 13, 2017

It looks like Windows Updates moved my Windows temp folder yesterday. Sigil worked fine on Friday.

I can recreate the problem by installing Sigil from scratch, open Preferences then click OK without changing anything. Then point my Windows temp folder to another folder. I then double-click on an epub file and it will instantly delete everything in the folder that the epub file was in, including all subfolders.

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor

The call to QTemporaryDir seems to fail quite often on Windows when provided with a template but never seem to do the same on Linux or OSX. No matter where your temp folder is located, it should be creating a new temporary directory with a sigil specific name that matches the template. And only that directory should ever be removed. I will try to detect when this happens and try to work around it.

Contributor

kevinhendricks commented Mar 13, 2017

The call to QTemporaryDir seems to fail quite often on Windows when provided with a template but never seem to do the same on Linux or OSX. No matter where your temp folder is located, it should be creating a new temporary directory with a sigil specific name that matches the template. And only that directory should ever be removed. I will try to detect when this happens and try to work around it.

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

It appears the problem for me was the Preferences screen hard-codes the temp_folder_path automatically when you open it. Then my temp folder moved without me knowing it, invalidating temp_folder_path.

UberBen commented Mar 13, 2017

It appears the problem for me was the Preferences screen hard-codes the temp_folder_path automatically when you open it. Then my temp folder moved without me knowing it, invalidating temp_folder_path.

@mihailim

This comment has been minimized.

Show comment
Hide comment
@mihailim

mihailim Mar 13, 2017

Contributor

C:\Users\bwinter\AppData\Local\Temp\4

Piping in to mention that the specific style of temporary directory above (the expected C:\Users\<username>\AppData\Local\Temp\ followed by a numeric subdirectory) sounds suspiciously like something the Remote Desktop Services session host would do.

Are you by any chance running Sigil under a Remote Desktop Services session? Or are you using some sort of distinct WinStation-based privilege separation system? (If you're not sure what I'm talking about, it's likely the answer is "no" :)

Can you also check under the registry key HKEY_LOCAL_MACHINE\System\CurrentControlSet\Control\Terminal Server and see if you have a DWORD entry there called PerSessionTempDir with a value of 1? Perhaps also check the similar DeleteTempDirsOnExit entry there.

See this entry on Raymond Chen's blog for a cursory mention of the topic.

This might be a red herring, but I've been bitten before by assumptions about the system environment that simply don't hold true under RDS sessions...

Contributor

mihailim commented Mar 13, 2017

C:\Users\bwinter\AppData\Local\Temp\4

Piping in to mention that the specific style of temporary directory above (the expected C:\Users\<username>\AppData\Local\Temp\ followed by a numeric subdirectory) sounds suspiciously like something the Remote Desktop Services session host would do.

Are you by any chance running Sigil under a Remote Desktop Services session? Or are you using some sort of distinct WinStation-based privilege separation system? (If you're not sure what I'm talking about, it's likely the answer is "no" :)

Can you also check under the registry key HKEY_LOCAL_MACHINE\System\CurrentControlSet\Control\Terminal Server and see if you have a DWORD entry there called PerSessionTempDir with a value of 1? Perhaps also check the similar DeleteTempDirsOnExit entry there.

See this entry on Raymond Chen's blog for a cursory mention of the topic.

This might be a red herring, but I've been bitten before by assumptions about the system environment that simply don't hold true under RDS sessions...

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

It is not running in Terminal Services. This is Windows 10 Enterprise Edition connnected to a domain with group policies enabled though.
PerSessionTempDir is 0 but DeleteTempDirsOnExit is 1

UberBen commented Mar 13, 2017

It is not running in Terminal Services. This is Windows 10 Enterprise Edition connnected to a domain with group policies enabled though.
PerSessionTempDir is 0 but DeleteTempDirsOnExit is 1

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor

When you open preference it should load that field up with the current temp location either one set by you earlier or the current system temp directory. It should be getting this from QDir::tempPath().

Contributor

kevinhendricks commented Mar 13, 2017

When you open preference it should load that field up with the current temp location either one set by you earlier or the current system temp directory. It should be getting this from QDir::tempPath().

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor

I double checked this and yes that is the default: Settings Store returns the current Qt tempPath() as determined by Qt runtime.

QString SettingsStore::tempFolderHome()
{
clearSettingsGroup();
return value(KEY_TEMP_FOLDER, QDir::tempPath()).toString();
}

This is the same value that Temp Directory will use. If you or something else is deleting the direcory returned by Qt's QDir::tempPath() you are going to run into trouble with all sorts of Qt applications that use temporary files as that is the default path.

Contributor

kevinhendricks commented Mar 13, 2017

I double checked this and yes that is the default: Settings Store returns the current Qt tempPath() as determined by Qt runtime.

QString SettingsStore::tempFolderHome()
{
clearSettingsGroup();
return value(KEY_TEMP_FOLDER, QDir::tempPath()).toString();
}

This is the same value that Temp Directory will use. If you or something else is deleting the direcory returned by Qt's QDir::tempPath() you are going to run into trouble with all sorts of Qt applications that use temporary files as that is the default path.

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

I just logged off and logged back in, and now my Windows temp folder is "C:\Users\bwinter\AppData\Local\Temp\2" and Sigil will not open anymore. I double-click on an epub file it deletes the whole folder the epub is in. Every time I log off my Windows temp folder changes again.

UberBen commented Mar 13, 2017

I just logged off and logged back in, and now my Windows temp folder is "C:\Users\bwinter\AppData\Local\Temp\2" and Sigil will not open anymore. I double-click on an epub file it deletes the whole folder the epub is in. Every time I log off my Windows temp folder changes again.

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor

Strange - I think Qt and most applications are built around the idea that the temp folder main directory itself should always exist and that we should be creating our own temp folders within it. And assuming no crash we should be removing our own temp folder created inside the main temp directory. You seem to be saying that the entire temp folder path is being removed and changed after every logout.

That is going to break Sigil no matter what.

Contributor

kevinhendricks commented Mar 13, 2017

Strange - I think Qt and most applications are built around the idea that the temp folder main directory itself should always exist and that we should be creating our own temp folders within it. And assuming no crash we should be removing our own temp folder created inside the main temp directory. You seem to be saying that the entire temp folder path is being removed and changed after every logout.

That is going to break Sigil no matter what.

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

Correct. Sigil works fine with the Windows temp folder changing every login until I open Preferences, which then hard-codes the temp folder and keeps it from working on next login.

Can you have Sigil check the validity of the temp_folder_path key on startup and remove it if it is invalid?

UberBen commented Mar 13, 2017

Correct. Sigil works fine with the Windows temp folder changing every login until I open Preferences, which then hard-codes the temp folder and keeps it from working on next login.

Can you have Sigil check the validity of the temp_folder_path key on startup and remove it if it is invalid?

@dougmassay

This comment has been minimized.

Show comment
Hide comment
@dougmassay

dougmassay Mar 13, 2017

Contributor

Why on earth is your temp location changing every login?

Contributor

dougmassay commented Mar 13, 2017

Why on earth is your temp location changing every login?

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor

Yes, and to harden things further I will prevent the removal of the specific Sigil temp folder if for any reason the QtTemporaryDir that is created thinks it is invalid.

Just pushed both fixes. They should be out in the upcoming release.

Contributor

kevinhendricks commented Mar 13, 2017

Yes, and to harden things further I will prevent the removal of the specific Sigil temp folder if for any reason the QtTemporaryDir that is created thinks it is invalid.

Just pushed both fixes. They should be out in the upcoming release.

@UberBen

This comment has been minimized.

Show comment
Hide comment
@UberBen

UberBen Mar 13, 2017

Thank you!

I checked the GPO settings based on @mihailim suggestion, and on our domain "Windows Components/Remote Desktop Services/Remote Desktop Session Host/Temporary folders/Do not use temporary folder per session" is disabled, which means the temp folder path changes every logon.

UberBen commented Mar 13, 2017

Thank you!

I checked the GPO settings based on @mihailim suggestion, and on our domain "Windows Components/Remote Desktop Services/Remote Desktop Session Host/Temporary folders/Do not use temporary folder per session" is disabled, which means the temp folder path changes every logon.

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor

@dougmassay
We should also probably change the meaning and use of the temp folder Preferences setting. Let it have just two states. An empty string meaning using whatever QDir::tempPath() returns at whatever moment Misc/TempFolder is invoked or set to some path manually by the user, and then always check if it exists when first launched and always check if it is valid before setting it.

In the PreferenceWidgets General Setting we could then simply change the Auto button to reset the string to the null string and have that mean use qDir::tempPath().

What do you think?

Contributor

kevinhendricks commented Mar 13, 2017

@dougmassay
We should also probably change the meaning and use of the temp folder Preferences setting. Let it have just two states. An empty string meaning using whatever QDir::tempPath() returns at whatever moment Misc/TempFolder is invoked or set to some path manually by the user, and then always check if it exists when first launched and always check if it is valid before setting it.

In the PreferenceWidgets General Setting we could then simply change the Auto button to reset the string to the null string and have that mean use qDir::tempPath().

What do you think?

@dougmassay

This comment has been minimized.

Show comment
Hide comment
@dougmassay

dougmassay Mar 13, 2017

Contributor

Whatever you think is best. Since that preference exists mainly to work around a fairly unique scenario on Macs, I'd be in favor of burying it somewhere. Maybe even change it to an environment variable override. I know that's not what was happening here, but we've already had some issues where people were changing it willy-nilly and simultaneously opening Sigil's scratch files with external programs (while Sigil was running).

But if you say we make it safe AND leave it as a user preference, I trust your judgement. ;)

Contributor

dougmassay commented Mar 13, 2017

Whatever you think is best. Since that preference exists mainly to work around a fairly unique scenario on Macs, I'd be in favor of burying it somewhere. Maybe even change it to an environment variable override. I know that's not what was happening here, but we've already had some issues where people were changing it willy-nilly and simultaneously opening Sigil's scratch files with external programs (while Sigil was running).

But if you say we make it safe AND leave it as a user preference, I trust your judgement. ;)

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 13, 2017

Contributor
Contributor

kevinhendricks commented Mar 13, 2017

@dougmassay

This comment has been minimized.

Show comment
Hide comment
@dougmassay

dougmassay Mar 13, 2017

Contributor

I'm fine with that.

Contributor

dougmassay commented Mar 13, 2017

I'm fine with that.

@kevinhendricks

This comment has been minimized.

Show comment
Hide comment
@kevinhendricks

kevinhendricks Mar 14, 2017

Contributor

Okay, I have converted SettingsStore, TempFolder, Utility, and General Settings to use and understand the placeholder "<SIGIL_DEFAULT_TEMP_HOME>" which will always represent current QDir::tempPath(). This coupled with earlier changes should now handle this issue in all its phases. So I will close this as "Fixed in Master".

Feel free to re-open this issue if needed after the next release if you still run into any troubles.

Thank you for your bug report and your willingness to help us track it down.

Contributor

kevinhendricks commented Mar 14, 2017

Okay, I have converted SettingsStore, TempFolder, Utility, and General Settings to use and understand the placeholder "<SIGIL_DEFAULT_TEMP_HOME>" which will always represent current QDir::tempPath(). This coupled with earlier changes should now handle this issue in all its phases. So I will close this as "Fixed in Master".

Feel free to re-open this issue if needed after the next release if you still run into any troubles.

Thank you for your bug report and your willingness to help us track it down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment