strip looses original file ownership and file permissions

strip looses original file ownership and file permissions

[Franz Sirl]

Hi,

I just verified that strip out of gas-990418 looses original ownership and 
permissions of a file.
This is on glibc-2.1.1pre2, Linux-2.2.6 (PPC).

Is this platform specific or does anybody else notice this?

Franz.

Re: strip looses original file ownership and file permissions

[Franz Sirl]

At 12:48 06.05.99 , Franz Sirl wrote:
>Hi,
>
>I just verified that strip out of gas-990418 looses original ownership and 
>permissions of a file.
>This is on glibc-2.1.1pre2, Linux-2.2.6 (PPC).
>
>Is this platform specific or does anybody else notice this?

After a quick browse through the source I came up with the following 
untested patch. Does it look right?

Franz.

Index: rename.c
===================================================================
RCS file: /cvs/binutils/binutils/binutils/rename.c,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 rename.c
--- rename.c    1999/05/03 07:29:10     1.1.1.1
+++ rename.c    1999/05/06 11:36:14
@@ -163,29 +163,26 @@ smart_rename (from, to, preserve_dates)
  #else
    /* Use rename only if TO is not a symbolic link and has
       only one hard link.  */
-  if (exists < 0 || (!S_ISLNK (s.st_mode) && s.st_nlink == 1))
+  if (exists == 0 && !S_ISLNK (s.st_mode) && s.st_nlink == 1)
      {
        ret = rename (from, to);
        if (ret == 0)
         {
-         if (exists)
-           {
-             /* Try to preserve the permission bits and ownership of
-                TO.  First get the mode right except for the setuid
-                bit.  Then change the ownership.  Then fix the setuid
-                bit.  We do the chmod before the chown because if the
-                chown succeeds, and we are a normal user, we won't be
-                able to do the chmod afterward.  We don't bother to
-                fix the setuid bit first because that might introduce
-                a fleeting security problem, and because the chown
-                will clear the setuid bit anyhow.  We only fix the
-                setuid bit if the chown succeeds, because we don't
-                want to introduce an unexpected setuid file owned by
-                the user running objcopy.  */
-             chmod (to, s.st_mode & 0777);
-             if (chown (to, s.st_uid, s.st_gid) >= 0)
-               chmod (to, s.st_mode & 07777);
-           }
+         /* Try to preserve the permission bits and ownership of
+            TO.  First get the mode right except for the setuid
+            bit.  Then change the ownership.  Then fix the setuid
+            bit.  We do the chmod before the chown because if the
+            chown succeeds, and we are a normal user, we won't be
+            able to do the chmod afterward.  We don't bother to
+            fix the setuid bit first because that might introduce
+            a fleeting security problem, and because the chown
+            will clear the setuid bit anyhow.  We only fix the
+            setuid bit if the chown succeeds, because we don't
+            want to introduce an unexpected setuid file owned by
+            the user running objcopy.  */
+         chmod (to, s.st_mode & 0777);
+         if (chown (to, s.st_uid, s.st_gid) >= 0)
+           chmod (to, s.st_mode & 07777);
         }
        else
         {

Re: strip looses original file ownership and file permissions

[Ian Lance Taylor]

   Date: Thu, 06 May 1999 13:44:47 +0200
   From: Franz Sirl <Franz.Sirl-kernel@lauterbach.com>

   >I just verified that strip out of gas-990418 looses original ownership and 
   >permissions of a file.
   >This is on glibc-2.1.1pre2, Linux-2.2.6 (PPC).
   >
   >Is this platform specific or does anybody else notice this?

   After a quick browse through the source I came up with the following 
   untested patch. Does it look right?

It doesn't look right to me.

We need to rename the file FROM to TO.  In the normal case of strip,
FROM is a temporary file, and TO is the original file which we are
stripping.  However, this function is also used in other cases.

If TO does not exist, we should just use rename.  This is not the
normal case of strip, but it happens in other cases.  Your patch
breaks that.  That seems to be only significant change in your patch.
Perhaps I am missing something.

I think the only way to reliably preserve ownership is to avoid using
rename.  Perhaps the code should be changed to call simple_copy when
the owner of the file differs from the effective uid.

Ian

Re: strip looses original file ownership and file permissions

[Franz Sirl]

At 18:27 06.05.99 , Ian Lance Taylor wrote:
>    Date: Thu, 06 May 1999 13:44:47 +0200
>    From: Franz Sirl <Franz.Sirl-kernel@lauterbach.com>
>
>    >I just verified that strip out of gas-990418 looses original 
> ownership and
>    >permissions of a file.
>    >This is on glibc-2.1.1pre2, Linux-2.2.6 (PPC).
>    >
>    >Is this platform specific or does anybody else notice this?
>
>    After a quick browse through the source I came up with the following
>    untested patch. Does it look right?
>
>It doesn't look right to me.
>
>We need to rename the file FROM to TO.  In the normal case of strip,
>FROM is a temporary file, and TO is the original file which we are
>stripping.  However, this function is also used in other cases.
>
>If TO does not exist, we should just use rename.  This is not the
>normal case of strip, but it happens in other cases.  Your patch
>breaks that.  That seems to be only significant change in your patch.
>Perhaps I am missing something.
>
>I think the only way to reliably preserve ownership is to avoid using
>rename.  Perhaps the code should be changed to call simple_copy when
>the owner of the file differs from the effective uid.

I think my patch produces the behavior that the comments are suggesting. 
The old code was bogus anyway, cause it tried to chmod/chown "to" after 
rename with values from a non-existing (exists != 0, this is really a 
bad-named variable, it's named the opposite of it's meaning) file.

Franz.

Re: strip looses original file ownership and file permissions

[Ian Lance Taylor]

   Date: Thu, 06 May 1999 18:54:16 +0200
   From: Franz Sirl <Franz.Sirl-kernel@lauterbach.com>

   At 18:27 06.05.99 , Ian Lance Taylor wrote:
   >    Date: Thu, 06 May 1999 13:44:47 +0200
   >    From: Franz Sirl <Franz.Sirl-kernel@lauterbach.com>
   >
   >    >I just verified that strip out of gas-990418 looses original 
   > ownership and
   >    >permissions of a file.
   >    >This is on glibc-2.1.1pre2, Linux-2.2.6 (PPC).
   >    >
   >    >Is this platform specific or does anybody else notice this?
   >
   >    After a quick browse through the source I came up with the following
   >    untested patch. Does it look right?
   >
   >It doesn't look right to me.
   >
   >We need to rename the file FROM to TO.  In the normal case of strip,
   >FROM is a temporary file, and TO is the original file which we are
   >stripping.  However, this function is also used in other cases.
   >
   >If TO does not exist, we should just use rename.  This is not the
   >normal case of strip, but it happens in other cases.  Your patch
   >breaks that.  That seems to be only significant change in your patch.
   >Perhaps I am missing something.
   >
   >I think the only way to reliably preserve ownership is to avoid using
   >rename.  Perhaps the code should be changed to call simple_copy when
   >the owner of the file differs from the effective uid.

   I think my patch produces the behavior that the comments are suggesting. 
   The old code was bogus anyway, cause it tried to chmod/chown "to" after 
   rename with values from a non-existing (exists != 0, this is really a 
   bad-named variable, it's named the opposite of it's meaning) file.

You're right.  I applied the appended patch, which I think should fix
the problem.

Ian

Index: rename.c
===================================================================
RCS file: /cvs/binutils/binutils/binutils/rename.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 rename.c
--- rename.c	1999/05/03 07:29:10	1.1.1.1
+++ rename.c	1999/05/06 18:02:05
@@ -139,17 +139,17 @@
      const char *to;
      int preserve_dates;
 {
-  int exists;
+  boolean exists;
   struct stat s;
   int ret = 0;
 
-  exists = lstat (to, &s);
+  exists = lstat (to, &s) == 0;
 
 #if defined (_WIN32) && !defined (__CYGWIN32__)
   /* Win32, unlike unix, will not erase `to' in `rename(from, to)' but
      fail instead.  Also, chown is not present.  */
 
-  if (exists == 0)
+  if (exists)
     remove (to);
 
   ret = rename (from, to);
@@ -163,7 +163,7 @@
 #else
   /* Use rename only if TO is not a symbolic link and has
      only one hard link.  */
-  if (exists < 0 || (!S_ISLNK (s.st_mode) && s.st_nlink == 1))
+  if (! exists || (!S_ISLNK (s.st_mode) && s.st_nlink == 1))
     {
       ret = rename (from, to);
       if (ret == 0)

Re: strip looses original file ownership and file permissions

[Franz Sirl]

At 20:03 06.05.99 , Ian Lance Taylor wrote:
>    I think my patch produces the behavior that the comments are suggesting.
>    The old code was bogus anyway, cause it tried to chmod/chown "to" after
>    rename with values from a non-existing (exists != 0, this is really a
>    bad-named variable, it's named the opposite of it's meaning) file.
>
>You're right.  I applied the appended patch, which I think should fix
>the problem.

Ok, this works fine, thanks.

Btw, have you ever looked at the patch in 
http://sourceware.cygnus.com/ml/gas2/1999/msg00041.html ? Is it the correct 
fix for the make check FAIL on powerpc-linux-gnu?

Franz.

Re: strip looses original file ownership and file permissions

[Ian Lance Taylor]

   Date: Thu, 06 May 1999 20:42:53 +0200
   From: Franz Sirl <Franz.Sirl-kernel@lauterbach.com>

   Btw, have you ever looked at the patch in 
   http://sourceware.cygnus.com/ml/gas2/1999/msg00041.html ? Is it the correct 
   fix for the make check FAIL on powerpc-linux-gnu?

No, I don't think it is.  I think this is a problem in the glibc
dynamic linker.  (I looked at glibc 2.0.7; I don't know about other
versions.)

The GNU linker does not ordinarily create a PT_PHDR program segment
for a shared library.  This follows the lead of the Solaris linker.

When the glibc 2.0.7 dynamic linker does not find a PT_PHDR program
segment, it assumes that the program headers may be found at the start
of the first PT_LOAD segment.  However, ELF does not require this.
The comment in the glibc sources (in dl-load.c) says this:

	/* There was no PT_PHDR specified.  We need to find the phdr in the
           load image ourselves.  We assume it is in fact in the load image
           somewhere, and that the first load command starts at the
           beginning of the file and thus contains the ELF file header.  */

However, I believe the dynamic linker should be able to operate
without having the program headers in the load segment at all.

What your patch changes is whether the linker puts the program headers
at the start of the first PT_LOAD segment.  Without your patch, the
linker does not put the program headers in loadable memory.  With your
patch, it does.

Since with the current script the program headers are not loaded, the
dynamic linker will wind up looking at nonsensical memory for the
program headers, and it will fail.  However, the dynamic linker does
know how to find the correct program headers by reading the file.
However, after reading the program headers, it throws them away, and
uses the copy which it expects to find in the file.  If there is no
PT_PHDR segment, it should instead keep and use its own copy.

Ian