107641 - on Linux CopyTo won't copy read-only files or broken symlinks[FIX]

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Garth Smedley]

In nsLocalFileUnix.cpp the function nsLocalFile::CopyTo creates the new file
with the same permissions as the old file. If the old file is read-only it means
the new file will be read-only and you won't be able to copy to it.

Suggest making files created by CopyTo have write permission for the owner,
something like:

rv = newFile->Create(NORMAL_FILE_TYPE, myPerms | S_IWRITE);

Comment 1

[sairuh (rarely reading bugmail)]

bryner?

Comment 2

[Brian Ryner (not reading)]

good suggestion.  -> 0.9.8.

Comment 3

[Brian Ryner (not reading)]

pete, any interest in taking this bug?

Comment 4

[Pete Collins (MDG)]

Taking bug

Comment 5

[Garth Smedley]

Attachment: patch for unix file and directory copy problems

This is a patch for copying read-only files and directories on linux. It also
fixes a problem with recursive copy of directories, each sub-dir was being
copied twice, the first containing only the second.

Comment 6

[Pete Collins (MDG)]

Attachment: patch i came up with

Comment 7

[Pete Collins (MDG)]

Ok, my patch is incomplete

I forgot the dir creates.

However, there is still a problem even with your patch Garth.

If you have a readonly dir with a bunch of readonly sub dirs, the sub dirs are
not copied.

I am working in this now

The only difference in patches here is i am setting the new files/dirs perm bits
to the originals
which is the way copy should work. You shouldn't change permissions bits on a copy.

--pete

Comment 8

[Garth Smedley]

Attachment: newer patch

oops submitted an old patch. Here is one that does copy read-only subfolders,
but does not restore the permissions after the copy. ( I agree it really ought
to. )

Comment 9

[Pete Collins (MDG)]

Attachment: restores permissions and behaves like cp -r

Comment 10

[Pete Collins (MDG)]

Well, if you are not root, you can't stat a dir that is read only.
You need execute to open it up.
 
So if you want' to do a copy of a dir that is dr--------

You will only copy the dir and *not* it's contents.
This is the same behavior of cp -r as well.

Garth see if this updated patch works for you.


--pete

Comment 11

[Pete Collins (MDG)]

Attachment: fixing other problems like copyTo failing on broken symlinks

Comment 12

[Pete Collins (MDG)]

Attachment: removing comment and tabs in nsLocalFileUnix.h

Comment 13

[Pete Collins (MDG)]

Ok, some of these fixes for copyTo i did over the summer as part of the nsIFIle
reorg
work but i never checked it in.

attacchment 65441 fixes the currently broken copyTo.

I'll see if i can get some review love for when the tree opens.

cc'ing Brendan and Doug

--pete

Comment 14

[Doug Turner (:dougt)]

blizzard, can you review this patch?

Comment 15

[Christopher Blizzard (:blizzard)]

Comment on attachment 65441 [details] [diff] [review]
removing comment and tabs in nsLocalFileUnix.h

r/sr=blizzard

Comment 16

[Pete Collins (MDG)]

Brendan can you sr this?

Thanks

--pete

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 65441 [details] [diff] [review]
removing comment and tabs in nsLocalFileUnix.h

>Index: nsLocalFileUnix.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/io/nsLocalFileUnix.cpp,v
>retrieving revision 1.80
>diff -u -r1.80 nsLocalFileUnix.cpp
>--- nsLocalFileUnix.cpp	2002/01/12 01:21:40	1.80
>+++ nsLocalFileUnix.cpp	2002/01/17 18:02:47
>@@ -633,8 +633,11 @@
>         // get the dirs old permissions
>         if (NS_FAILED(rv = GetPermissions(&oldPerms)))
>             return rv;
>-        if (NS_FAILED(rv = newParent->Create(DIRECTORY_TYPE, oldPerms)))
>+        if (NS_FAILED(rv = newParent->Create(DIRECTORY_TYPE, S_IRWXU)))
>             return rv;
>+        // set permissions back to original
>+        if (NS_FAILED(rv = newParent->SetPermissions(oldPerms)))
>+            return rv;
>     } else {    // dir exists lets try to use leaf
>         nsXPIDLCString leafName;
>         if (NS_FAILED(rv = GetLeafName(getter_Copies(leafName))))
>@@ -645,7 +648,10 @@
>             return rv;
>         if (dirCheck) 
>             return NS_ERROR_FILE_ALREADY_EXISTS; // dest exists
>-        if (NS_FAILED(rv = newParent->Create(DIRECTORY_TYPE, oldPerms)))
>+        if (NS_FAILED(rv = newParent->Create(DIRECTORY_TYPE, S_IRWXU)))
>+            return rv;
>+        // set permissions back to original
>+        if (NS_FAILED(rv = newParent->SetPermissions(oldPerms)))
>             return rv;
>     }
> 

I don't see why either of these changes is needed -- Create makes no links in
the newly created directory, so why do you need to create with 0700, then chmod
back to oldPerms?

>@@ -659,20 +665,15 @@
>         rv = dirIterator->GetNext((nsISupports**)getter_AddRefs(entry));
>         if (NS_FAILED(rv)) 
>             continue;
>-        if (NS_FAILED(rv = entry->IsDirectory(&dirCheck)))
>-            return rv;
>         if (NS_FAILED(rv = entry->IsSymlink(&isSymlink)))
>             return rv;
>+        if (NS_FAILED(rv = entry->IsDirectory(&dirCheck)))
>+            return rv;
>         if (dirCheck && !isSymlink) {

Old nit, sorry for not asking earlier, or fixing myself: would not isDir match
isSymlink and be a better name all around than dirCheck?

>         PRUint32 myPerms;
>         GetPermissions(&myPerms);
> 
>-        // create the new file with the same permissions
>-        rv = newFile->Create(NORMAL_FILE_TYPE, myPerms);
>+        // create the new file with user rwx permissions
>+        rv = newFile->Create(NORMAL_FILE_TYPE, S_IRWXU);

Ok, *here* I see the need to create a regular file with 0700, write it, and
finally fix the perms to be what were requested.

>         if (NS_FAILED(rv)) {
>             NS_RELEASE(newFile);
>             return rv;
>@@ -772,16 +772,33 @@
>         PRInt32     modeFlags = myPerms;
>         PRFileDesc *newFD;
> 
>-        rv = newFile->OpenNSPRFileDesc(openFlags, modeFlags, &newFD);
>+        rv = newFile->OpenNSPRFileDesc(openFlags, S_IRWXU, &newFD);
>         if (NS_FAILED(rv)) {
>             NS_RELEASE(newFile);
>             return rv;
>         }
> 
>+        // set the permissions of newFile to original files 
>+        if (NS_FAILED(rv = newFile->SetPermissions(myPerms)))
>+            return rv;

You sure you want to do this before the read/write loop?  I don't know whether
nsLocalFileUnix.cpp is supposed to work on NFS "local files", but as NFS is
stateless, I suspect you might have trouble writing after revoking write
permission.

>+        PRBool specialFile;
>+        if (NS_FAILED(rv = IsSpecial(&specialFile)))
>+            return rv;
>+        if (specialFile) {
>+#ifdef DEBUG
>+            printf("1: Operation not supported: %s\n", mPath.get());
>+#endif

1.  Use PR_LOG, not printf, or don't print anything here (is this really
necessary for others?  Could it be DEBUG_petejc or even removed once you've
tested?).
2.  Use a better message:  what does the "1: " mean above?

>+        if (stat(mPath.get(), &mCachedStat) == -1) {
>+            // try lstat it may be a symlink
>+            if (lstat(mPath.get(), &mCachedStat) == -1) {
>+#ifdef DEBUG
>+                printf("stat failed %s\n", mPath.get());
>+#endif

Same 2 comments here.

>+                return NS_ERROR_FAILURE;

And should this not be return NSRESULT_FOR_ERRNO()?

>+            }
>+        }
>+        mHaveCachedStat = PR_TRUE;
>+        return NS_OK;
>     }
>-
> };
> 
> #endif /* _nsLocalFileUNIX_H_ */

/be

Comment 18

[Pete Collins (MDG)]

> I don't see why either of these changes is needed -- Create makes no links in
> the newly created directory, so why do you need to create with 0700, then chmod
> back to oldPerms?

Yes, i was being paranoid there. Removed

> Old nit, sorry for not asking earlier, or fixing myself: would not isDir match
> isSymlink and be a better name all around than dirCheck?

dirCheck is a generic name for a PRBool that is being used in various contexts.
IsDirectory, Equals, Exists etc. I did this to be efficient and i don't see any
need to change it.

> You sure you want to do this before the read/write loop?  

I am very sure i want to do this before the read/write loop. This prevents
copyTo from breaking on socket files like '.X11-unix/X0'.

> I don't know whether
> nsLocalFileUnix.cpp is supposed to work on NFS "local files", but as NFS is
> stateless, I suspect you might have trouble writing after revoking write
> permission.

Nope, NFS mounted file writes work quite wonderfully.

> 1.  Use PR_LOG, not printf, or don't print anything here (is this really
> necessary for others?  Could it be DEBUG_petejc or even removed once you've
> tested?).

No, i am being consistant w/ code higher above that encounters uncopyable files.

> 2.  Use a better message:  what does the "1: " mean above?

Sorry, the 1: was a typo. I removed it. 
The message is keeping with the convention of cp || cp -r 
So if an uncopyable file is encountered, we print out the same message as good
o'l cp in DEBUG builds. This is to keep w/ common convention. 

I removed the stat printf, that was for my own debugging and am returning
NSRESULT_FOR_ERRNO() instead of NS_ERROR_FAILURE.

Revised patch forthcoming.

--pete

Comment 19

[Pete Collins (MDG)]

Attachment: Fixes for Brendans review

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 66638 [details] [diff] [review]
Fixes for Brendans review

>@@ -633,7 +633,10 @@
>         // get the dirs old permissions
>         if (NS_FAILED(rv = GetPermissions(&oldPerms)))
>             return rv;
>-        if (NS_FAILED(rv = newParent->Create(DIRECTORY_TYPE, oldPerms)))
>+        if (NS_FAILED(rv = newParent->Create(DIRECTORY_TYPE, S_IRWXU)))
>+            return rv;
>+        // set permissions back to original
>+        if (NS_FAILED(rv = newParent->SetPermissions(oldPerms)))
>             return rv;

You missed this one -- there were two cases of a mkdir;chmod where just mkdir
with the ultimate perms will do.

>+        if (NS_FAILED(rv = entry->IsDirectory(&dirCheck)))
>+            return rv;

I hear you on dirCheck, I do that too -- I just wanted to reassure you that if
you use separate PRBool locals with precise names, any modern optimizing
compiler will unify them if their lifetimes are disjoint.  So suit yourself,
but don't fear inefficiency -- the only downside of multiple, disjoint,
precisely named vars is the code clutter of declaring 'em.

dougt: are we still on an anti-printf embedding/embedability kick?

pete, thanks -- sr=[e-mail] with the above mkdir;chmod => mkdir fix.

/be

Comment 21

[Pete Collins (MDG)]

Attachment: Final revision cleanup of 66638

Comment 22

[Pete Collins (MDG)]

Checked in. Thanks everyone.

--pete

Comment 23

[Adam D. Moss]

FYI, I'm now getting this (linux, glibc2.1):

In file included from ../../dist/include/xpcom/nsLocalFile.h:46,
                 from nsComponentManager.cpp:56:
../../dist/include/xpcom/nsLocalFileUnix.h: In method `nsresult
nsLocalFile::FillStatCache()':
../../dist/include/xpcom/nsLocalFileUnix.h:102: `lstat' undeclared (first use
this function)
../../dist/include/xpcom/nsLocalFileUnix.h:102: (Each undeclared identifier is
reported only once
../../dist/include/xpcom/nsLocalFileUnix.h:102: for each function it appears in.)

Comment 24

[Pete Collins (MDG)]

NAME
       stat, fstat, lstat - get file status

SYNOPSIS
       #include <sys/stat.h>
       #include <unistd.h>

I just added <unistd.h> which was missing. 

Seeing now if this fixes the problem.  

--pete

Comment 25

[Pete Collins (MDG)]

Hrmm, this is weird i can compile a sample using lstat on linux glib 1.2.6 no
problems.

--pete

Comment 26

[david avery]

glibc 2.2.4 only defines lstat if:

 __USE_BSD , __USE_XOPEN_EXTENDED or
__USE_LARGEFILE64 are defined

Comment 27

[Pete Collins (MDG)]

Well, lstat *has* been used for some time now in nsLocalFileUnix.cpp
implementation file.

seawood said that you need -D_BSD_SOURCE defeined for it to work.

But that is already defined.  I will add _USE_BSD if we still fail on the next spin.

--pete

Comment 28

[david avery]

moving the lstat call out of the header file should have fixed the bustage

Comment 29

[Adam D. Moss]

Builds good on my system.  Ta.