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

Offline schickb

  • Single posting newcomer
  • *
  • Posts: 9
Linux file save permission bug
« 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.

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #1 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.

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #2 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;


Offline schickb

  • Single posting newcomer
  • *
  • Posts: 9
Re: Linux file save permission bug
« Reply #3 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?
« Last Edit: March 31, 2008, 10:42:36 pm by schickb »

Offline DrewBoo

  • Multiple posting newcomer
  • *
  • Posts: 110
Re: Linux file save permission bug
« Reply #4 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;

« Last Edit: April 01, 2008, 12:12:32 am by DrewBoo »

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #5 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.

Offline DrewBoo

  • Multiple posting newcomer
  • *
  • Posts: 110
Re: Linux file save permission bug
« Reply #6 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.

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: Linux file save permission bug
« Reply #7 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.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline Ceniza

  • Developer
  • Lives here!
  • *****
  • Posts: 1441
    • CenizaSOFT
Re: Linux file save permission bug
« Reply #8 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.

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #9 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();

Offline Ceniza

  • Developer
  • Lives here!
  • *****
  • Posts: 1441
    • CenizaSOFT
Re: Linux file save permission bug
« Reply #10 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 and SetNamedSecurityInfo. 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?

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: Linux file save permission bug
« Reply #11 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 and SetNamedSecurityInfo. 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.

Offline Biplab

  • Developer
  • Lives here!
  • *****
  • Posts: 1874
    • Biplab's Blog
Re: Linux file save permission bug
« Reply #12 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. :)
« Last Edit: April 07, 2008, 06:26:56 pm 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 #13 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: ).

Offline Biplab

  • Developer
  • Lives here!
  • *****
  • Posts: 1874
    • Biplab's Blog
Re: Linux file save permission bug
« Reply #14 on: April 07, 2008, 06:35:21 pm »
The Save() function without BOM should not be used at all. :)
Be a part of the solution, not a part of the problem.