Code::Blocks Forums

User forums => General (but related to Code::Blocks) => Topic started by: schickb on March 30, 2008, 03:21:17 am

Title: Linux file save permission bug
Post by: schickb on March 30, 2008, 03:21:17 am
Using CB 8.02 on Ubuntu 8.10 beta.

I opened a python file with permissions of rwxr-xr-x in CB. I edited the file and saved it, and found the permissions set to rw-r--r--. Each time I save the file, CB seems to trash the current permissions and set them back to the default umask.

BTW, CB is by far the best open source IDE I've tried. The symbol engine for C is great.
Title: Re: Linux file save permission bug
Post by: Jenna on March 30, 2008, 03:23:32 pm
I can confirm this.
I think it happens, because C::B creates a backup-file, saves the changes to a new file and then replaces the original-file with the new on, but it does not touch the umask. So the standard umask will also be used.
Title: Re: Linux file save permission bug
Post by: Jenna on March 30, 2008, 07:32:21 pm
I didn't find an easy way to get and set the permissions of a file with wxWidgets.

So I created a short patch that copies and renames the original file, instead of creating a new (empty) one.

It might not be the most elegant way, I think, but it works and it does not change any other function.

Code
--- codeblocks-1.0svn.orig/src/sdk/filemanager.cpp      2008-03-01 09:23:55.000000000 +0100
+++ codeblocks-1.0svn.work/src/sdk/filemanager.cpp      2008-03-30 19:18:01.000000000 +0200
@@ -309,6 +309,10 @@
     wxString tempName(name + _T(".cbTemp"));
     do
     {
+        if(wxRenameFile(name, tempName) == false)
+            return false;
+        if(wxCopyFile(tempName, name) ==false)
+            return false;
         wxFile f(tempName, wxFile::write);
         if(!f.IsOpened())
             return false;

Title: Re: Linux file save permission bug
Post by: schickb on March 31, 2008, 10:40:47 pm
Thanks. I haven't build CB from source yet, but I guess I will since this bug makes editing python scripts annoying. Or I see that your site has .deps, have you applied this patch there?
Title: Re: Linux file save permission bug
Post by: DrewBoo on March 31, 2008, 11:04:03 pm
I didn't find an easy way to get and set the permissions of a file with wxWidgets.

So I created a short patch that copies and renames the original file, instead of creating a new (empty) one.

It might not be the most elegant way, I think, but it works and it does not change any other function.

Have you checked if wxCopyFile copies the attributes?

If it does, this tweak of your patch would work.

(I'm not changing your patch just for the sake of changing it.  I'm concerned that the added code in the patch above could succeed with wxFileRename and fail with wxCopyFile and then your file is mysteriously missing.  See what I'm saying?)

Code
--- codeblocks-1.0svn.orig/src/sdk/filemanager.cpp      2008-03-01 09:23:55.000000000 +0100
+++ codeblocks-1.0svn.work/src/sdk/filemanager.cpp      2008-03-30 19:18:01.000000000 +0200
@@ -309,6 +309,8 @@
     wxString tempName(name + _T(".cbTemp"));
     do
     {
+        if(wxCopyFile(name, tempName) ==false)
+            return false;
         wxFile f(tempName, wxFile::write);
         if(!f.IsOpened())
             return false;

Title: Re: Linux file save permission bug
Post by: Jenna on April 01, 2008, 12:06:43 am
@schickb
yes, my patch is applied to the C::B-version in my repo.

@DrewBoo
you are right, your patch works without problems.

I haven't tested copying the file with wxWidgets, because copying it on commandline with "cp" does not copy the file-permissions, and I believed that wxWidgets uses the same mechanism the OS uses.
That's not the case , but I'm not sure if we can rely on the fact that it works with future releases of wxWidgets or (for example) on Mac.
I cannot test it on other OS's expect on windows. Nevertheless it's the dev's decision if they want to fix this bug and how they do it.

The problem that renaming works, but copying not , is not a real problem I think, because the renamed file should still be there. If not something really went wrong.
Title: Re: Linux file save permission bug
Post by: DrewBoo on April 01, 2008, 12:19:58 am
I haven't tested copying the file with wxWidgets, because copying it on commandline with "cp" does not copy the file-permissions, and I believed that wxWidgets uses the same mechanism the OS uses.

Ha.  I didn't know cp wouldn't work.  I would have made the same assumption.   :shock:

The problem that renaming works, but copying not , is not a real problem I think, because the renamed file should still be there. If not something really went wrong.

Not a realistic problem, agreed.  My work experience has tweaked me to always think of ways code could possibly break. :lol:    Particularly when digging my hands into other peoples' code.
Title: Re: Linux file save permission bug
Post by: thomas on April 01, 2008, 10:17:20 am
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.
Title: Re: Linux file save permission bug
Post by: Ceniza on April 06, 2008, 10:10:18 pm
What about this simple code to query the file permissions of the original file and assign them to the file-after-safe-saving?

Code
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
...
struct stat fileStats;
stat("file.cpp", &fileStats);
// vodoo magic to do safe file saving
chmod("file.cpp", fileStats.st_mode);

That should do the trick, and it could even work on Mac OS.
Title: Re: Linux file save permission bug
Post by: Jenna on April 06, 2008, 11:44:25 pm
Hi Ceniza,

it's simple, but it works.
At least on my Laptop with debian sid (64-bit) with utf-8 as standard-encoding (in C::B and filesystem).
Not tested with other combinations.
On windows the same bug exists and persists.
If I have special rights for some users, they all are set to default (everyone can do everything, all added users get removed) on my w2k-box.

Nevertheless here is your idea as working patch.
Compiles on linux and w2k.

It should not break the "ultra-paranoid custom file save function that Code::Blocks uses".

Code
--- codeblocks-1.0svn.orig/src/sdk/filemanager.cpp      2008-04-02 23:11:44.000000000 +0200
+++ codeblocks-1.0svn.work/src/sdk/filemanager.cpp      2008-04-06 23:22:29.000000000 +0200
@@ -278,6 +278,12 @@
         if(!f.IsOpened())
             return false;

+        // take file permissions from original file ...
+        struct stat fileStats;
+        stat(name.mb_str(), &fileStats);
+        // ... and give them to the new one
+        chmod(tempName.mb_str(), fileStats.st_mode);
+
         if(f.Write(data, len) != len)
         {
             f.Close();
@@ -313,6 +319,12 @@
         if(!f.IsOpened())
             return false;

+        // take file permissions from original file ...
+        struct stat fileStats;
+        stat(name.mb_str(), &fileStats);
+        // ... and give them to the new one
+        chmod(tempName.mb_str(), fileStats.st_mode);
+
         if(WriteWxStringToFile(f, data, encoding, bom) == false)
         {
             f.Close();
Title: Re: Linux file save permission bug
Post by: Ceniza on April 07, 2008, 10:30:57 am
Playing with ACLs looks like a lot more trouble. Searching the MSDN, it seems the right functions to play with are GetNamedSecurityInfo (http://msdn2.microsoft.com/en-us/library/aa446645(VS.85).aspx) and SetNamedSecurityInfo (http://msdn2.microsoft.com/en-us/library/aa379579(VS.85).aspx). Furthermore, it should be checked, at runtime, that those functions are available under the version of Windows CB is running on (opening advapi32.dll and calling GetProcAddress for the needed functions). Anyone up to the task?
Title: Re: Linux file save permission bug
Post by: Jenna on April 07, 2008, 02:51:55 pm
Playing with ACLs looks like a lot more trouble. Searching the MSDN, it seems the right functions to play with are GetNamedSecurityInfo (http://msdn2.microsoft.com/en-us/library/aa446645(VS.85).aspx) and SetNamedSecurityInfo (http://msdn2.microsoft.com/en-us/library/aa379579(VS.85).aspx). Furthermore, it should be checked, at runtime, that those functions are available under the version of Windows CB is running on (opening advapi32.dll and calling GetProcAddress for the needed functions). Anyone up to the task?

I surely will look for this, but I don't know when I find the time to do.

Another problem what came in my mind are ACLs on linux. I think a simple stat and chmod will not be enough to copy/restore them.

Another way to do copying could be to:
 1. write the changed file to disk (as backup-file)
 2. if there was no error -> write the changed data to the original-file.
 3. if there was again no error -> delete the backup-file

that should work and there is no need to deal with file-permissions, and there should not be any risk of losing data as long as wxWidgets reports correctly if there was an error or not.
Title: Re: Linux file save permission bug
Post by: Biplab on April 07, 2008, 06:23:22 pm
This patch should solve the issue. I've tested this on Windows. :)

Code
Index: src/sdk/filemanager.cpp
===================================================================
--- src/sdk/filemanager.cpp (revision 4999)
+++ src/sdk/filemanager.cpp (working copy)
@@ -299,29 +299,35 @@
         return WriteWxStringToFile(f, data, encoding, bom);
     }
 
-    if(platform::windows)
-    {
-        wxFile f;
-        if(!f.Open(name, wxFile::read_write))
-            return false;
-    }
+    wxFile f;
+    if (!f.Open(name, wxFile::read_write))
+        return false;
 
     wxString tempName(name + _T(".cbTemp"));
     do
     {
-        wxFile f(tempName, wxFile::write);
-        if(!f.IsOpened())
+        if (!wxCopyFile(name, tempName))
+        {
+            f.Close();
             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);
+    return true;
 }
 
 bool FileManager::ReplaceFile(const wxString& old_file, const wxString& new_file)


@Thomas,

If you know the exact version of wx which has the buggy wxCopyFile() implementation, then we can specify the exact version users should avoid. :)
Title: Re: Linux file save permission bug
Post by: Jenna on April 07, 2008, 06:29:45 pm
There are two "Save"-functions, one with BOM and one without. It should be changed in both (I forgot it in my first patch too  :oops: ).
Title: Re: Linux file save permission bug
Post by: Biplab on April 07, 2008, 06:35:21 pm
The Save() function without BOM should not be used at all. :)
Title: Re: Linux file save permission bug
Post by: Jenna on April 07, 2008, 07:08:57 pm
Then it should perhaps get removed or commented out.
Title: Re: Linux file save permission bug
Post by: Jenna 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.
Title: Re: Linux file save permission bug
Post by: Biplab 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. :)
Title: Re: Linux file save permission bug
Post by: Biplab 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??
Title: Re: Linux file save permission bug
Post by: 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. 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).
Title: Re: Linux file save permission bug
Post by: Biplab 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. :)
Title: Re: Linux file save permission bug
Post by: Jenna 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 (http://forums.codeblocks.org/index.php/topic,8388.msg61925.html#msg61925)
Title: Re: Linux file save permission bug
Post by: Carjay 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.
Title: Re: Linux file save permission bug
Post by: Biplab 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
Title: Re: Linux file save permission bug
Post by: Jenna 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.