About pseudo's chmod

Submitted by Robert Yang on July 29, 2016, 7:38 a.m. | Patch ID: 128223

Details

Message ID 579B07F9.9000708@windriver.com
State New
Headers show

Commit Message

Robert Yang July 29, 2016, 7:38 a.m.
On 07/05/2016 09:10 PM, Mark Hatle wrote:
> On 7/5/16 5:23 AM, Robert Yang wrote:
>> Hi,
>>
>> When run "chmod 0444 <file>" under pseudo, it would always adds
>> write permission for real file (and w + x for dir), which means that
>> it runs as "chmod 0644 <file>". It does this on real file, not record
>> this on pseudo's database. Here are the code from pseudo:
>>
>> /* Root can read and write files, and enter directories which have no
>>    * read, write, or execute permissions.  (But can't execute files without
>>    * execute permissions!)
>>    *
>>    * A non-root user can't.
>>    *
>>    * When doing anything which actually writes to the filesystem, we add in
>>    * the user read/write/execute bits.  When storing to the database, though,
>>    * we mask out any such bits which weren't in the original mode.
>>    */
>> #define PSEUDO_FS_MODE(mode, isdir) (((mode) | S_IRUSR | S_IWUSR | ((isdir) ?
>> S_IXUSR : 0)) & ~(S_IWGRP | S_IWOTH))
>> #define PSEUDO_DB_MODE(fs_mode, user_mode) (((fs_mode) & ~0722) | ((user_mode &
>> 0722)))
>>
>> It has a side effect for -dbg pkgs if the source files foo.c's mode is 0444:
>> 1) bitbake foo
>> 2) Edit rpm-native
>> 3) bitbake foo
>>
>> After the first bitake foo, we will see that foo.c in foo-dbg is 0444, but
>> after the second bitbake foo, foo.c in foo-dbg will be 0644, because the first
>> build has changed src file foo.c's mode to 0644, this is incorrect.
>>
>> I have two suggestions on it:
>> 1) Don't add more permissions when chmod(e.g., don't change 0444 -> 0644),
>>      The user can add it clearly if a file/dir really needs that.
>
> As noted above, we have to adjust the permissions to writable, or we can not
> make various changes later.  When working as the 'root' user, permissions are
> basically ignored.  So a non-writable file is still writable.  The only way to
> emulate this is to make the actual file writable.
>
> I don't understand how/why on a second run the 0644 is showing up though.

Hi Mark,

It got 0644 but not 0444 in the second build was because pseudo's unlink
doesn't take core of hard links, for example:
$ umask 0022
$ touch file1
$ ln file1 file2
$ chmod 0777 file1
$ ls -l file1 file2
We can see that both file1 and file2's mode is 0777.

But if we remove file1:
$ rm -f file1
$ ls file2
Now file2's mode is 0644 since the info had been removed from database.

After talked with RP online, there isn't any file systems that can support
different modes on different references for the same inode, so pseudo's
chmod should update all the references' mode, in another word, it should
not remove the info from database if links count is greater than 1.

Here is a patch for pseudo to fix the problem, I'm testing it locally now,
will send out sooner:

effect.\n", path);
     }

// Robert

> Unless the pseudo database is wiped -- or the debug commands are not clearing
> the split directories before writing into them, it should be creating new files
> with the new pseudo database that follows the same semantics.
>
>> 2) This mainly affects do_package task AFAIK, the code is:
>>               if not cpath.islink(file):
>>                   os.link(file, fpath)
>>                   fstat = cpath.stat(file)
>>                   os.chmod(fpath, fstat.st_mode)
>>                   os.chown(fpath, fstat.st_uid, fstat.st_gid)
>>
>>      Another solution is checking mode before run chmod, if we really need
>>      run chmod, then copy the file rather than link.
>>
>> Any suggestion is appreciated.
>>
>> The following recipes in oe-core have this issue:
>> blktool
>> coreutils
>> e2fsprogs
>> gnutls
>> guile
>> gzip
>> less
>> lsof
>> mtools
>> opensp
>> parted
>> screen
>> tcp-wrappers
>>
>
>

Patch hide | download patch | download mbox

diff --git a/ports/unix/guts/unlinkat.c b/ports/unix/guts/unlinkat.c
index e723a01..a0ff685 100644
--- a/ports/unix/guts/unlinkat.c
+++ b/ports/unix/guts/unlinkat.c
@@ -36,8 +36,9 @@ 
     if (rc == -1) {
         return rc;
     }
+
     msg = pseudo_client_op(OP_MAY_UNLINK, 0, -1, dirfd, path, &buf);
-   if (msg && msg->result == RESULT_SUCCEED)
+   if (msg && msg->result == RESULT_SUCCEED && buf.st_nlink == 1)
         old_db_entry = 1;
  #ifdef PSEUDO_NO_REAL_AT_FUNCTIONS
     rc = real_unlink(path);
@@ -52,6 +53,8 @@ 
         } else {
             pseudo_client_op(OP_DID_UNLINK, 0, -1, -1, path, &buf);
         }
+   } else if (buf.st_nlink > 1) {
+       pseudo_debug(PDBGF_FILE, "not remove ino %lu from database since its 
links count is greater than 1.\n", buf.st_ino);
     } else {
         pseudo_debug(PDBGF_FILE, "unlink on <%s>, not in database, no 

Comments

Robert Yang July 29, 2016, 7:40 a.m.
On 07/29/2016 03:38 PM, Robert Yang wrote:
>
>
> On 07/05/2016 09:10 PM, Mark Hatle wrote:
>> On 7/5/16 5:23 AM, Robert Yang wrote:
>>> Hi,
>>>
>>> When run "chmod 0444 <file>" under pseudo, it would always adds
>>> write permission for real file (and w + x for dir), which means that
>>> it runs as "chmod 0644 <file>". It does this on real file, not record
>>> this on pseudo's database. Here are the code from pseudo:
>>>
>>> /* Root can read and write files, and enter directories which have no
>>>    * read, write, or execute permissions.  (But can't execute files without
>>>    * execute permissions!)
>>>    *
>>>    * A non-root user can't.
>>>    *
>>>    * When doing anything which actually writes to the filesystem, we add in
>>>    * the user read/write/execute bits.  When storing to the database, though,
>>>    * we mask out any such bits which weren't in the original mode.
>>>    */
>>> #define PSEUDO_FS_MODE(mode, isdir) (((mode) | S_IRUSR | S_IWUSR | ((isdir) ?
>>> S_IXUSR : 0)) & ~(S_IWGRP | S_IWOTH))
>>> #define PSEUDO_DB_MODE(fs_mode, user_mode) (((fs_mode) & ~0722) | ((user_mode &
>>> 0722)))
>>>
>>> It has a side effect for -dbg pkgs if the source files foo.c's mode is 0444:
>>> 1) bitbake foo
>>> 2) Edit rpm-native
>>> 3) bitbake foo
>>>
>>> After the first bitake foo, we will see that foo.c in foo-dbg is 0444, but
>>> after the second bitbake foo, foo.c in foo-dbg will be 0644, because the first
>>> build has changed src file foo.c's mode to 0644, this is incorrect.
>>>
>>> I have two suggestions on it:
>>> 1) Don't add more permissions when chmod(e.g., don't change 0444 -> 0644),
>>>      The user can add it clearly if a file/dir really needs that.
>>
>> As noted above, we have to adjust the permissions to writable, or we can not
>> make various changes later.  When working as the 'root' user, permissions are
>> basically ignored.  So a non-writable file is still writable.  The only way to
>> emulate this is to make the actual file writable.
>>
>> I don't understand how/why on a second run the 0644 is showing up though.
>
> Hi Mark,
>
> It got 0644 but not 0444 in the second build was because pseudo's unlink
> doesn't take core of hard links, for example:
> $ umask 0022
> $ touch file1
> $ ln file1 file2
> $ chmod 0777 file1
> $ ls -l file1 file2
> We can see that both file1 and file2's mode is 0777.
>
> But if we remove file1:
> $ rm -f file1
> $ ls file2
> Now file2's mode is 0644 since the info had been removed from database.
>
> After talked with RP online, there isn't any file systems that can support
> different modes on different references for the same inode, so pseudo's

Here should be: "there isn't any file systems can ... *as far as we know*"

// Robert

> chmod should update all the references' mode, in another word, it should
> not remove the info from database if links count is greater than 1.
>
> Here is a patch for pseudo to fix the problem, I'm testing it locally now,
> will send out sooner:
>
> diff --git a/ports/unix/guts/unlinkat.c b/ports/unix/guts/unlinkat.c
> index e723a01..a0ff685 100644
> --- a/ports/unix/guts/unlinkat.c
> +++ b/ports/unix/guts/unlinkat.c
> @@ -36,8 +36,9 @@
>      if (rc == -1) {
>          return rc;
>      }
> +
>      msg = pseudo_client_op(OP_MAY_UNLINK, 0, -1, dirfd, path, &buf);
> -   if (msg && msg->result == RESULT_SUCCEED)
> +   if (msg && msg->result == RESULT_SUCCEED && buf.st_nlink == 1)
>          old_db_entry = 1;
>   #ifdef PSEUDO_NO_REAL_AT_FUNCTIONS
>      rc = real_unlink(path);
> @@ -52,6 +53,8 @@
>          } else {
>              pseudo_client_op(OP_DID_UNLINK, 0, -1, -1, path, &buf);
>          }
> +   } else if (buf.st_nlink > 1) {
> +       pseudo_debug(PDBGF_FILE, "not remove ino %lu from database since its
> links count is greater than 1.\n", buf.st_ino);
>      } else {
>          pseudo_debug(PDBGF_FILE, "unlink on <%s>, not in database, no
> effect.\n", path);
>      }
>
> // Robert
>
>> Unless the pseudo database is wiped -- or the debug commands are not clearing
>> the split directories before writing into them, it should be creating new files
>> with the new pseudo database that follows the same semantics.
>>
>>> 2) This mainly affects do_package task AFAIK, the code is:
>>>               if not cpath.islink(file):
>>>                   os.link(file, fpath)
>>>                   fstat = cpath.stat(file)
>>>                   os.chmod(fpath, fstat.st_mode)
>>>                   os.chown(fpath, fstat.st_uid, fstat.st_gid)
>>>
>>>      Another solution is checking mode before run chmod, if we really need
>>>      run chmod, then copy the file rather than link.
>>>
>>> Any suggestion is appreciated.
>>>
>>> The following recipes in oe-core have this issue:
>>> blktool
>>> coreutils
>>> e2fsprogs
>>> gnutls
>>> guile
>>> gzip
>>> less
>>> lsof
>>> mtools
>>> opensp
>>> parted
>>> screen
>>> tcp-wrappers
>>>
>>
>>
Seebs July 29, 2016, 4:02 p.m.
On 29 Jul 2016, at 2:38, Robert Yang wrote:

> It got 0644 but not 0444 in the second build was because pseudo's 
> unlink
> doesn't take core of hard links, for example:
> $ umask 0022
> $ touch file1
> $ ln file1 file2
> $ chmod 0777 file1
> $ ls -l file1 file2
> We can see that both file1 and file2's mode is 0777.

> But if we remove file1:
> $ rm -f file1
> $ ls file2
> Now file2's mode is 0644 since the info had been removed from 
> database.

I don't get that result. Before the remove, I see:

sqlite> select * from files;
1|/home/seebs/pseudo/f1|2054|2757274|0|0|33279|0|0
2|/home/seebs/pseudo/f2|2054|2757274|0|0|33279|0|0

So both files have the correct information stored for them. This is
because chmod-type operations on files update everything with the same
device and inode.

> After talked with RP online, there isn't any file systems that can 
> support
> different modes on different references for the same inode, so 
> pseudo's
> chmod should update all the references' mode

Yes.

> in another word, it should
> not remove the info from database if links count is greater than 1.

This doesn't seem right to me. I can't produce any failures from the
current code that match the behavior you're describing; updates like
chmod always propogate to all the links.

On the other hand, if f1 *already exists*, and was not being tracked
by pseudo, then we do indeed see a similar problem. The underlying 
file's
mode is actually changed.

I do not think it would be a good idea at all to stop removing entries 
from the database when they become invalid. It seems to me that if we're 
hard linking to a file inside pseudo, we should probably be tracking 
that file. Although if we track the file, now we'll just continue to see 
it as mode 777, because we changed the mode of the file.

Consider, for a moment, what happens if you do this *without* pseudo 
involved. Unpack an archive containing a file "foo", mode 444.

$ ln foo bar
$ chmod 777 bar
$ rm bar
$ ls -l foo

You'll find that foo's mode is now 0777. So it'd change either way; the
difference is that, when pseudo's doing this, it masks out group and
other write bits on the filesystem. (Because we don't want other people 
to be able to overwrite things in the build tree just because they'd be 
writeable on the target filesystem.)

-s
Robert Yang Aug. 1, 2016, 5:57 a.m.
Hi Peter,

Thanks for the reply.

On 07/30/2016 12:02 AM, Seebs wrote:
> On 29 Jul 2016, at 2:38, Robert Yang wrote:
>
>> It got 0644 but not 0444 in the second build was because pseudo's unlink
>> doesn't take core of hard links, for example:
>> $ umask 0022
>> $ touch file1
>> $ ln file1 file2
>> $ chmod 0777 file1
>> $ ls -l file1 file2
>> We can see that both file1 and file2's mode is 0777.
>
>> But if we remove file1:
>> $ rm -f file1
>> $ ls file2
>> Now file2's mode is 0644 since the info had been removed from database.
>
> I don't get that result. Before the remove, I see:
>
> sqlite> select * from files;
> 1|/home/seebs/pseudo/f1|2054|2757274|0|0|33279|0|0
> 2|/home/seebs/pseudo/f2|2054|2757274|0|0|33279|0|0
>
> So both files have the correct information stored for them. This is
> because chmod-type operations on files update everything with the same
> device and inode.

Sorry, the steps were wrong, they should be:
1) Out of pseudo:
    $ umask 0022
    $ touch file1
2) Under pseudo:
    $ ln file1 file2
    $ chmod 777 file2
    $ ls -l file1 file2
    We can see that both file1 and file2's mode is 0777.
    But if we remove file2:
    $ rm -f file2
    $ ls file1
    Now file1's permission is 0755, not 0777 or 0644, it should be 0777 here.

>
>> After talked with RP online, there isn't any file systems that can support
>> different modes on different references for the same inode, so pseudo's
>> chmod should update all the references' mode
>
> Yes.
>
>> in another word, it should
>> not remove the info from database if links count is greater than 1.
>
> This doesn't seem right to me. I can't produce any failures from the
> current code that match the behavior you're describing; updates like
> chmod always propogate to all the links.
>
> On the other hand, if f1 *already exists*, and was not being tracked
> by pseudo, then we do indeed see a similar problem. The underlying file's
> mode is actually changed.
>
> I do not think it would be a good idea at all to stop removing entries from the
> database when they become invalid. It seems to me that if we're hard linking to
> a file inside pseudo, we should probably be tracking that file. Although if we

Yes, I agree that we need track the src file when hard link.

// Robert

> track the file, now we'll just continue to see it as mode 777, because we
> changed the mode of the file.
>
> Consider, for a moment, what happens if you do this *without* pseudo involved.
> Unpack an archive containing a file "foo", mode 444.
>
> $ ln foo bar
> $ chmod 777 bar
> $ rm bar
> $ ls -l foo
>
> You'll find that foo's mode is now 0777. So it'd change either way; the
> difference is that, when pseudo's doing this, it masks out group and
> other write bits on the filesystem. (Because we don't want other people to be
> able to overwrite things in the build tree just because they'd be writeable on
> the target filesystem.)
>
> -s
Seebs Aug. 1, 2016, 8:42 a.m.
On 1 Aug 2016, at 0:57, Robert Yang wrote:

> Sorry, the steps were wrong, they should be:
> 1) Out of pseudo:
>    $ umask 0022
>    $ touch file1
> 2) Under pseudo:
>    $ ln file1 file2
>    $ chmod 777 file2
>    $ ls -l file1 file2
>    We can see that both file1 and file2's mode is 0777.
>    But if we remove file2:
>    $ rm -f file2
>    $ ls file1
>    Now file1's permission is 0755, not 0777 or 0644, it should be 0777 
> here.

The short answer is: If you aren't tracking a file in pseudo, we don't 
make promises about its behavior. Normally, we don't touch them, but if 
there's hard links to them that are being touched, well. And having a 
hard link that is tracked, and another that isn't, is probably 
impossible to support. I definitely don't want to keep database entries 
for files that have been deleted, that way lies madness and possible 
database corruption; for instance, if we do that, and you make a new 
file of the same type, it'll show up as having those permissions, with 
only a path-mismatch warning in the database to suggest what went wrong.

I would say that the probable correct answer is either "make the 
original file be tracked" or "don't use hard links in this case".

Note that, under older pseudo, the file would have ended up 0777. The 
behavior of silently masking out 022 from underlying filesystem 
permissions is entirely intentional. During some debugging quite a while 
back, we discovered a quirk in oe-core, plus a strange behavior of
GNU tar, which between them resulted in some sstage directories getting
unpacked with mode 0777.

And we *really* don't want the build system generating files which other 
users can modify, especially not in stuff that's intended to go into a 
root filesystem. So stripping out those bits in the underlying 
filesystem is intentional.

If you were actually root, yes, the original file would have its mode 
changed to 0777. But we should never be *caring* about the mode bits on
raw files outside of pseudo, except that we want them to allow owner
write permissions and not allow group or other write permissions. If the
file's permissions matter to the build system or generated stuff, the
file should be tracked by pseudo.

-s