Author Topic: Linux file save permission bug  (Read 21337 times)

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #15 on: April 07, 2008, 07:08:57 pm »
Then it should perhaps get removed or commented out.

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #16 on: April 08, 2008, 08:32:35 am »
The Save() function without BOM should not be used at all. :)

It is used by tinyXML in base/tinyxml/tinywxuni.cpp in line 54.

Offline Biplab

  • Developer
  • Lives here!
  • *****
  • Posts: 1874
    • Biplab's Blog
Re: Linux file save permission bug
« Reply #17 on: April 08, 2008, 03:41:34 pm »
The Save() function without BOM should not be used at all. :)

It is used by tinyXML in base/tinyxml/tinywxuni.cpp in line 54.


The main difference is that the Save() function which accepts wxString and BOM is able to convert & save correctly to Multi-byte encoded file. So if anyone wishes to save a file beyond 1-byte encoding, (s)he needs to use the newer one. Older one may be used to save any files with 1-byte encoding.

AFAIK, the call inside TinyXML is used to save C::B's project files, layout files, etc. They are saved as UTF-8 which the old one is able to handle. It should also be replaced with the newer one.

Actually while the Unicode handling portion of C::B was updated, the api changes were reflected in core items. Now I see I've missed one. :)
Be a part of the solution, not a part of the problem.

Offline Biplab

  • Developer
  • Lives here!
  • *****
  • Posts: 1874
    • Biplab's Blog
Re: Linux file save permission bug
« Reply #18 on: April 24, 2008, 05:38:25 pm »
@Thomas,

What do you think about the patch? Shall I clean it up and go ahead to commit??
Be a part of the solution, not a part of the problem.

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: Linux file save permission bug
« Reply #19 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. For example, the "keep the backup file" bit will keep it renamed as tempfile, if I read the patch correctly. 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 :)
Also, I'm not sure if copy-write-replace can cope with rename-write-replace performance-wise. 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. You must be sure that this doesn't break project files and config files (so please test before changing that).
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline Biplab

  • Developer
  • Lives here!
  • *****
  • Posts: 1874
    • Biplab's Blog
Re: Linux file save permission bug
« Reply #20 on: April 25, 2008, 09:11:42 am »
Looking at the last version of that patch, I don't think it'll be 100% good.

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. :)


For example, the "keep the backup file" bit will keep it renamed as tempfile, if I read the patch correctly.

I need to look into this.

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 :)

I'll restore original behaviour.

Also, I'm not sure if copy-write-replace can cope with rename-write-replace performance-wise.

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.

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.

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. :)

You must be sure that this doesn't break project files and config files (so please test before changing that).

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. :)
« Last Edit: April 25, 2008, 09:14:03 am by Biplab »
Be a part of the solution, not a part of the problem.

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #21 on: May 25, 2008, 10:58:05 pm »
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)

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;
}

It's absolutely necessary not to open the file we want to write into in readwrite-mode !!

See my post here

Carjay

  • Guest
Re: Linux file save permission bug
« Reply #22 on: May 25, 2008, 11:43:56 pm »
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.

Offline Biplab

  • Developer
  • Lives here!
  • *****
  • Posts: 1874
    • Biplab's Blog
Re: Linux file save permission bug
« Reply #23 on: June 01, 2008, 06:14:06 pm »
@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)

Regards,

Biplab
Be a part of the solution, not a part of the problem.

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #24 on: June 01, 2008, 06:38:33 pm »
@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:
Have you checked if wxCopyFile copies the attributes?
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.

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.