User forums > General (but related to Code::Blocks)
Linux file save permission bug
Biplab:
--- Quote from: thomas on April 24, 2008, 06:38:05 pm ---Looking at the last version of that patch, I don't think it'll be 100% good.
--- End quote ---
I should agree that I didn't test it fully. That's why I wrote that I need to do clean-up before I commit. :)
--- Quote from: thomas on April 24, 2008, 06:38:05 pm ---For example, the "keep the backup file" bit will keep it renamed as tempfile, if I read the patch correctly.
--- End quote ---
I need to look into this.
--- Quote from: thomas on April 24, 2008, 06:38:05 pm ---The Windows-specific workaround for the broken readonly flag is now enabled on all platforms, which doesn't do much harm, but I don't understand what it's for :)
--- End quote ---
I'll restore original behaviour.
--- Quote from: thomas on April 24, 2008, 06:38:05 pm ---Also, I'm not sure if copy-write-replace can cope with rename-write-replace performance-wise.
--- End quote ---
I don't think so. There are two write operations in both the cases; in the present code & in my patch. :)
1. In the present code it's write and then replace.
2. In the new patch, it's copy and then write. But there is no replace.
So I don't think it'll affect the performance.
--- Quote from: thomas on April 24, 2008, 06:38:05 pm ---I'm not sure about that BOM story and tinyxml either. It may be that I just chose "non-BOM" back in the old days because it wasn't needed, or it may be that there was a problem, I don't remember... so long ago.
--- End quote ---
BOM story is applicable while certain encodings are used. If BOM is used, detection of encoding becomes much faster. :)
This new Save() routine was added to save multi-byte encoded files properly. IIRC, the Non-BOM routine used to fail to save multi-byte encoded files properly in many cases. :)
Also note that the Save() routine without BOM still contains old code. I haven't touched it while I made changes to improve Unicode support. :)
--- Quote from: thomas on April 24, 2008, 06:38:05 pm ---You must be sure that this doesn't break project files and config files (so please test before changing that).
--- End quote ---
IIRC, they still uses old code; that's the Save() routine without BOM one. I didn't touch this code earlier. :)
I'm not in a hurry to apply this patch as it involves a critical portion of C::B. I'll post a better patch here so that other users may comment on it before I commit it. :)
Jenna:
Here is the patch I actually use for the binaries in my repo:
--- Code: ------ codeblocks-1.0svn.orig/src/sdk/filemanager.cpp 2008-05-04 14:14:54.000000000 +0200
+++ codeblocks-1.0svn.work/src/sdk/filemanager.cpp 2008-05-10 12:46:55.000000000 +0200
@@ -264,29 +264,48 @@
return f.Write(data, len);
}
+ wxString tempName(name + _T(".cbTemp"));
+
+ wxFile f_tmp(tempName, wxFile::write);
+ if(!f_tmp.IsOpened())
+ return false;
+
+ if(f_tmp.Write(data, len) != len)
+ {
+ f_tmp.Close();
+ wxRemoveFile(tempName);
+ return false;
+ }
+
if(platform::windows) // work around broken Windows readonly flag
{
- wxFile f;
- if(!f.Open(name, wxFile::read_write))
+ wxFile f(name, wxFile::read_write);
+ if(!f.IsOpened())
return false;
}
-
- wxString tempName(name + _T(".cbTemp"));
do
{
- wxFile f(tempName, wxFile::write);
+ wxFile f(name, wxFile::write);
if(!f.IsOpened())
return false;
-
if(f.Write(data, len) != len)
{
f.Close();
- wxRemoveFile(tempName);
return false;
}
}while(false);
- return ReplaceFile(name, tempName);
+ if (Manager::IsAppShuttingDown())
+ {
+ // app shut down, forget delayed deletion
+ wxRemoveFile(tempName);
+ }
+ else
+ {
+ // issue a delayed deletion of the back'd up (old) file
+ delayedDeleteThread.Queue(new DelayedDelete(tempName));
+ }
+ return true;
}
bool FileManager::Save(const wxString& name, const wxString& data, wxFontEncoding encoding, bool bom)
@@ -296,32 +315,53 @@
wxFile f(name, wxFile::write);
if(!f.IsOpened())
return false;
+
return WriteWxStringToFile(f, data, encoding, bom);
}
- if(platform::windows)
+ wxString tempName(name + _T(".cbTemp"));
+
+ wxFile f_tmp(tempName, wxFile::write);
+ if(!f_tmp.IsOpened())
+ return false;
+
+ if(WriteWxStringToFile(f_tmp, data, encoding, bom) == false)
+ {
+ f_tmp.Close();
+ wxRemoveFile(tempName);
+ return false;
+ }
+
+ if(platform::windows) // work around broken Windows readonly flag
{
- wxFile f;
- if(!f.Open(name, wxFile::read_write))
+ wxFile f(name, wxFile::read_write);
+ if(!f.IsOpened())
return false;
}
-
- wxString tempName(name + _T(".cbTemp"));
do
{
- wxFile f(tempName, wxFile::write);
+ wxFile f(name, wxFile::write);
if(!f.IsOpened())
return false;
if(WriteWxStringToFile(f, data, encoding, bom) == false)
{
f.Close();
- wxRemoveFile(tempName);
return false;
}
}while(false);
- return ReplaceFile(name, tempName);
+ if (Manager::IsAppShuttingDown())
+ {
+ // app shut down, forget delayed deletion
+ wxRemoveFile(tempName);
+ }
+ else
+ {
+ // issue a delayed deletion of the back'd up (old) file
+ delayedDeleteThread.Queue(new DelayedDelete(tempName));
+ }
+ return true;
}
bool FileManager::ReplaceFile(const wxString& old_file, const wxString& new_file)
--- End code ---
That leads to the following functions (I only post the function with BOM, but it's identically to the other one except the way the string is written to the files):
--- Code: ---bool FileManager::Save(const wxString& name, const wxString& data, wxFontEncoding encoding, bool bom)
{
if(wxFileExists(name) == false) // why bother if we don't need to
{
wxFile f(name, wxFile::write);
if(!f.IsOpened())
return false;
return WriteWxStringToFile(f, data, encoding, bom);
}
wxString tempName(name + _T(".cbTemp"));
wxFile f_tmp(tempName, wxFile::write);
if(!f_tmp.IsOpened())
return false;
if(WriteWxStringToFile(f_tmp, data, encoding, bom) == false)
{
f_tmp.Close();
wxRemoveFile(tempName);
return false;
}
if(platform::windows) // work around broken Windows readonly flag
{
wxFile f(name, wxFile::read_write);
if(!f.IsOpened())
return false;
}
do
{
wxFile f(name, wxFile::write);
if(!f.IsOpened())
return false;
if(WriteWxStringToFile(f, data, encoding, bom) == false)
{
f.Close();
return false;
}
}while(false);
if (Manager::IsAppShuttingDown())
{
// app shut down, forget delayed deletion
wxRemoveFile(tempName);
}
else
{
// issue a delayed deletion of the back'd up (old) file
delayedDeleteThread.Queue(new DelayedDelete(tempName));
}
return true;
}
--- End code ---
It's absolutely necessary not to open the file we want to write into in readwrite-mode !!
See my post here
Carjay:
Ah, great timing.
I stumbled across the original permission bug yesterday (also with python files) and searching on the forums brought me to this thread.
I applied the original patch of biplab yesterday. But it wasn't until today that I noticed the non-truncating behaviour when I received a python error in a non-existing line in one of my files. :shock:
I have now applied your patch from above and it works for me so far (Kubuntu Feisty x86_64).
Thanks for the effort.
Biplab:
@Jens,
IMO, there is a problem with your patch.
The workflow of your patch is-
1) Write the new contents to a temporary file.
2) Write the new contents to the original file.
3) Delete the file created in step 1.
The problem is, once step 2 is started, the original file is lost. In case of any error, there is no way of recovering the original file.
I've added your modification of delayed delete to my original patch and planning to commit it later this week. This is what my patch looks right now. I need to apply it to the other Save function.
--- Code: ---Index: src/sdk/filemanager.cpp
===================================================================
--- src/sdk/filemanager.cpp (revision 5083)
+++ src/sdk/filemanager.cpp (working copy)
@@ -309,19 +309,42 @@
wxString tempName(name + _T(".cbTemp"));
do
{
- wxFile f(tempName, wxFile::write);
- if(!f.IsOpened())
+ if (!wxCopyFile(name, tempName))
+ {
return false;
+ }
+ wxFile f(name, wxFile::write);
+ if (!f.IsOpened())
+ {
+ return false;
+ }
if(WriteWxStringToFile(f, data, encoding, bom) == false)
{
f.Close();
- wxRemoveFile(tempName);
+ // Keep the backup file as the original file has been destroyed
return false;
}
+ else
+ {
+ wxRemoveFile(tempName);
+ }
+
+ f.Close();
+
}while(false);
- return ReplaceFile(name, tempName);
+ if (Manager::IsAppShuttingDown())
+ {
+ // app shut down, forget delayed deletion
+ wxRemoveFile(tempName);
+ }
+ else
+ {
+ // issue a delayed deletion of the back'd up (old) file
+ delayedDeleteThread.Queue(new DelayedDelete(tempName));
+ }
+ return true;
}
bool FileManager::ReplaceFile(const wxString& old_file, const wxString& new_file)
--- End code ---
Regards,
Biplab
Jenna:
@Biblap
I know about the problem with the original file-content (my first patch uses rename and copy).
The only way (to try) to get it back would be to undo changes and then save the file with another name.
I do not use copy in my patch because Thomas wrote that copying might be unsafe:
--- Quote from: thomas on April 01, 2008, 10:17:20 am ---
--- Quote from: DrewBoo on March 31, 2008, 11:04:03 pm ---Have you checked if wxCopyFile copies the attributes?
--- End quote ---
If you do that, be aware that you give up some safety. wxCopyFile and wxTempFile are (even though the documentation claims so) not safe.
There is (at least) one situation that I've already encountered too, in which wxWidgets will delete both the old and the new file, making you lose your data. The ultra-paranoid custom file save function that Code::Blocks uses was written as reaction to that.
--- End quote ---
So I decided to write the new content (the content the user wants to have, as I think) to a tempfile and only if that works to overwrite the original file.
If the last step does not work at least the tempfile will be still there and contain the changed code.
It would perhaps be good to check if a tempfile exists (maybe because of former error) and ask the user if he wants to overwrite it, because if first saving does not work and the user clicks save a second time the tempfile might be the last remaining file with correct data.
Or better to create the tempfiles name (or the extension) with the actual timestamp included to make sure it will not be overwritten after an error happened.
Navigation
[0] Message Index
[*] Previous page
Go to full version