Skip Menu |
 

This queue is for tickets about the File-Slurp CPAN distribution.

Report information
The Basics
Id: 92401
Status: resolved
Priority: 0/
Queue: File-Slurp

People
Owner: cwhitener [...] gmail.com
Requestors: pause [...] tlinx.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 9999.19
Fixed in: (no value)



Subject: test "t/perms.t" is invalid; umask test broken w/ACLs
Download (untitled) / with headers
text/plain 795b
t/perms.t sets umask to 027 writes a file and checks that file's mode is 0640. This is not guaranteed to be true on file systems with ACL's. It is best illustrated by setting a default ACL on the direct in which you are creating the test file. That sets the ACL on any newly created file. Give 'other' any access other than none (like 'r-x' instead of '---'). Then any file created will give "other" read access for non-executables. The defaults established by the file systems override process defaults like umask. Ishtar:> umask 027 Ishtar:> touch zzz Show quoted text
> ll zzz
-rw-rw-r--+ 1 0 Jan 22 21:01 zzz ---why?--- Show quoted text
> lsacl zzz
[u::rw-,u:law:rwx,g::rwx,g:lawgroup:rwx,m::rw-,o::r--] zzz (which it inherited from the directory) -- which specifies its own ACL and a default acl for created files.
BTW, I marked this 'critical', as it prevents File::Slurp from installing (unless done manually).
Download (untitled) / with headers
text/plain 270b
I'm not sure that I see a way to accurately test the file permissions unless I move this test to the xt/author section so that it's not tested for installs on servers where we can't guarantee there aren't ACLs in place. Do you have any other suggestions? Thanks, Chase
Subject: Re: [rt.cpan.org #92401] test "t/perms.t" is invalid; umask test broken w/ACLs
Date: Thu, 04 Oct 2018 20:53:50 -0700
To: bug-File-Slurp [...] rt.cpan.org
From: "L. A. Walsh" <pause [...] tlinx.org>
Download (untitled) / with headers
text/plain 750b
On 10/4/2018 7:01 PM, Chase Whitener via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=92401 > > > I'm not sure that I see a way to accurately test the file permissions unless I move this test to the xt/author section so that it's not tested for installs on servers where we can't guarantee there aren't ACLs in place. > > Do you have any other suggestions? > > Thanks, > Chase > >
I'm running this on Win7SP1, which is a desktop/workstation/client machine. I'm not sure how this is different from installs on servers. Not sure what the xt/author section is or how it prevents this problem since I'm not installing on a server in the first place. What does this have to do with servers vs. clients? (sorry, I don't understand)
Download (untitled) / with headers
text/plain 337b
If you're concerned about building on Windows, then I'm even further confused as this test never even runs on Windows. https://ci.appveyor.com/project/genio/slurp/build/job/vtrmufja5wv9jf1p#L224 I'll be happy to revisit this, but I don't know what we can do since we're already skipping this entire test on the platform. Thanks, Chase
Subject: Re: [rt.cpan.org #92401] test "t/perms.t" is invalid; umask test broken w/ACLs
Date: Fri, 05 Oct 2018 09:56:42 -0700
To: bug-File-Slurp [...] rt.cpan.org
From: "L. A. Walsh" <pause [...] tlinx.org>
Download (untitled) / with headers
text/plain 706b
On 10/5/2018 4:48 AM, Chase Whitener via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=92401 > > > If you're concerned about building on Windows, then I'm even further confused as this test never even runs on Windows. > https://ci.appveyor.com/project/genio/slurp/build/job/vtrmufja5wv9jf1p#L224 > > I'll be happy to revisit this, but I don't know what we can do since we're already skipping this entire test on the platform. > > Thanks, > Chase > >
When i run on windows its under cygwin. I also run on my 64bit linux box. I don't remember which architecture this test was run on, but if you test doesn't run on windows, then it seems pretty obvious that it wasn't on windows, right?
Download (untitled) / with headers
text/plain 420b
On 2018-10-05 09:56:52, LAWALSH wrote: Show quoted text
> When i run on windows its under cygwin. I also run on my 64bit linux > box. > I don't remember which architecture this test was run on, but if you > test > doesn't run on windows, then it seems pretty obvious that it wasn't > on > windows, > right?
You said: "I'm running this on Win7SP1", so you can hardly blame Chase for being confused and asking for more information.
Subject: Re: [rt.cpan.org #92401] test "t/perms.t" is invalid; umask test broken w/ACLs
Date: Fri, 05 Oct 2018 12:07:27 -0700
To: bug-File-Slurp [...] rt.cpan.org
From: "L. A. Walsh" <pause [...] tlinx.org>
Download (untitled) / with headers
text/plain 837b
On 10/5/2018 10:11 AM, Karen Etheridge via RT wrote: Show quoted text
> You said: "I'm running this on Win7SP1", so you can hardly blame Chase > for being confused and asking for more information.
I wasn't aware I was assigning blame. I fail to see how assigning blame fixes the problem. It doesn't seem related and only had the effect of derailing Chase's ability to clarify what servers had to do with this. If you aren't trying to solve the problem, why are posting on this bug? Looking at the original post, the output looks like what one might get from a linux machine. Indeed, Ishtar is my linux devel. machine (among other roles). So there is still the question, outstanding, that you derailed. So are you going to answer the question or are you here just sough confusion and doubt? What does this have to do with servers vs. clients?
Download (untitled) / with headers
text/plain 1.8k
It seems this entire conversation has been derailed over one word. Forget "server" vs. "client". You're attempting to install the module on a machine that uses ACLs (it doesn't matter if said machine is a server or a client or a workstation or a phone). That attempt to install failed because the test in question tries to set and test permissions on a file. I said "I'm not sure that I see a way to accurately test the file permissions unless I move this test to the xt/author section so that it's not tested for installs on servers where we can't guarantee there aren't ACLs in place. Do you have any other suggestions?" Please replace the word "servers" with "machines" in that. Do you have any suggestions about _how_ we might test to see if the machine in question is using ACLs so that we can skip the tests in such a scenario? If not, I can't think of a better solution than to move that particular test into the xt/authors set of tests (which do not run on end-user installs). Thanks, Chase On Fri Oct 05 15:07:43 2018, LAWALSH wrote: Show quoted text
> On 10/5/2018 10:11 AM, Karen Etheridge via RT wrote:
> > You said: "I'm running this on Win7SP1", so you can hardly blame Chase > > for being confused and asking for more information.
> I wasn't aware I was assigning blame. I fail to see how assigning blame > fixes the problem. It doesn't seem related and only had the effect of > derailing > Chase's ability to clarify what servers had to do with this. If you aren't > trying to solve the problem, why are posting on this bug? > > Looking at the original post, the output looks like what one might get > from a linux machine. Indeed, Ishtar is my linux devel. machine (among > other roles). > > So there is still the question, outstanding, that you derailed. So are you > going to answer the question or are you here just sough confusion and doubt? > > What does this have to do with servers vs. clients?
Subject: Re: [rt.cpan.org #92401] test "t/perms.t" is invalid; umask test broken w/ACLs
Date: Fri, 05 Oct 2018 15:21:00 -0700
To: bug-File-Slurp [...] rt.cpan.org
From: "L. A. Walsh" <pause [...] tlinx.org>
Download (untitled) / with headers
text/plain 1.6k
On 10/5/2018 2:00 PM, Chase Whitener via RT wrote: Show quoted text
> "I'm not sure that I see a way to accurately test the file permissions unless I move this test to the xt/author section so that it's not tested for installs on servers where we can't guarantee there aren't ACLs in place. >
Ironically, the fact that you don't support windows has somewhat protected you from running into this problem earlier (not that I'm suggesting you should support native windows or not). Show quoted text
> Do you have any other suggestions?" >
--- The bit of axing the test sounds best in _this_ case only because the permissions on the file are incidental to the prime purpose of the module (IMO) which is to slurp them in/out. "Obviously" (ha), if permissions prevent reading or writing, you won't be able to slurp in or out, but testing mode bits related to a umask setting seems very incidental to your primary objective. Another failure case (not tested, but suspect) would be mounting FAT (and maybe exFAT) file systems on linux. They would not have the same types of permission bits, so depending on how detailed such testing is, I could easily see failure cases there. What you _could_ do, is test for read access before reading and write access before writing. It may be simple, but as near as I can tell, it's all your module would likely need, no? Anything else, be it permission bits, ACL's, or mandatory controls like in SELinux or similar (which comes installed by default on RedHat and Suse flavors of linux) would be OS specific permission controls that seem beyond what you would want to test (I think). Am I missing something? -linda
Download (untitled) / with headers
text/plain 2.9k
On Fri Oct 05 18:21:18 2018, LAWALSH wrote: Show quoted text
> On 10/5/2018 2:00 PM, Chase Whitener via RT wrote:
> > "I'm not sure that I see a way to accurately test the file > > permissions unless I move this test to the xt/author section so that > > it's not tested for installs on servers where we can't guarantee > > there aren't ACLs in place. > >
> Ironically, the fact that you don't support windows has somewhat > protected > you from running into this problem earlier (not that I'm suggesting > you > should > support native windows or not).
We _do_ support Windows. I'm not sure what's giving any impression otherwise. However, Windows cannot set file permissions in the same way that other OSes can. This particular test is for that one purpose only. The rest of the tests run on Windows just like every other OS. This in no way means we don't support Windows, just that such a test is pointless to try on Windows. Show quoted text
>
> > Do you have any other suggestions?" > >
> --- > The bit of axing the test sounds best in _this_ case only because > the permissions on the file are incidental to the prime purpose of > the > module > (IMO) which is to slurp them in/out. > > "Obviously" (ha), if permissions prevent reading or writing, you won't > be able to slurp in or out, but testing mode bits related to a umask > setting seems very incidental to your primary objective. Another > failure case (not tested, but suspect) would be mounting FAT (and > maybe > exFAT) file systems > on linux. They would not have the same types of permission bits, so > depending > on how detailed such testing is, I could easily see failure cases > there.
Again, this is one small detail about how this particular function operates. Testing all facets of the methods is what the test suite does. Slurping in files it tested hundreds of other ways on Windows; this one feature cannot be tested in that environment because the environment doesn't support the process. Show quoted text
> > What you _could_ do, is test for read access before reading and > write access > before writing. It may be simple, but as near as I can tell, it's all > your module would likely need, no? Anything else, be it permission > bits, ACL's, > or mandatory controls like in SELinux or similar (which comes > installed > by default on RedHat and Suse flavors of linux) would be OS specific > permission controls that seem beyond what you would want to test (I > think). >
We do test all of these things. I'm in no way suggesting that we do not testing, just that we skip this one portion of tests when we can't accurately test due to differences in how the OS operates. Show quoted text
> Am I missing something? > > -linda
This conversation seems to have again deviated from the original problem of how to detect when ACLs are present on the current file system that would prevent us from testing this portion of the functionality of the module. I'll leave this open for now until our next release when we'll move this particular test to the author test suite, mitigating any possible issues from leaving it in the mix. Thanks, Chase
Subject: Re: [rt.cpan.org #92401] test "t/perms.t" is invalid; umask test broken w/ACLs
Date: Fri, 05 Oct 2018 21:51:37 -0700
To: bug-File-Slurp [...] rt.cpan.org
From: "L. A. Walsh" <pause [...] tlinx.org>
Download (untitled) / with headers
text/plain 2.9k
On 10/5/2018 6:00 PM, Chase Whitener via RT wrote: Show quoted text
> > > This conversation seems to have again deviated from the original problem of how to detect when ACLs are present on the current file system that would prevent us from testing this portion of the functionality of the module. >
---- If at first you don't succeed... look for binaries 'chacl, getfacl setfacl' if on linux. I don't know of an OS independent way to check for acls, but solaris, cygwin linux are likely to have get/set facl. And see if they can be set or listed? For linux, another place to look for OS support would be /proc/config.gz: Show quoted text
> zgrep ACL /proc/config.gz
CONFIG_EXT4_FS_POSIX_ACL=y CONFIG_XFS_POSIX_ACL=y CONFIG_BTRFS_FS_POSIX_ACL=y CONFIG_FS_POSIX_ACL=y CONFIG_TMPFS_POSIX_ACL=y CONFIG_CIFS_ACL=y --- it shows the running kernel's ACL support Are those the type of things you are looking for to see if ACL's may be present?...unfortunately, linux specific. or something like this (practical test): a directory (named 'file') with an acl: Show quoted text
> chacl -l file
[u::rwx,g::rwx,g:Trusted\040Local\040Net\040Users:r-x,m::rwx,o::r-x/u::rwx,u:Media:rwx,g::r-x,g:Media:rwx,g:Trusted\040Local\040Net\040Users:r-x,m::rwx,o::r-x] file Show quoted text
> cd file > umask 777 > touch 77 (should be 000, no?) > umask 000 > touch 00 (should be rw+rw+rw, no?) > mkdir dir > chacl -l dir
[u::rwx,u:Media:rwx,g::r-x,g:Media:rwx,g:Trusted\040Local\040Net\040Users:r-x,m::rwx,o::r-x/u::rwx,u:Media:rwx,g::r-x,g:Media:rwx,g:Trusted\040Local\040Net\040Users:r-x,m::rwx,o::r-x] dir Show quoted text
> llg -a
total 16 drwxrwsr-x 2 Media Media 36 Oct 5 21:24 ./ drwxrwxr-x 13 Media Media 4096 Oct 5 21:25 ../ -rw-rw-r-- 1 law Media 0 Oct 5 21:23 00 -rw-rw-r-- 1 law Media 0 Oct 5 21:24 77 drwxrwsr-x 2 law Media 10 Oct 5 21:32 dir/ file 00 doesn't have 'write' for other (o::r-x) and file 77 still has access for user 'law' and group Media Another gotcha -- if parent has set groupid set, that will propagate to child dirs on xfs by default, though there are ways to change that behavior: Show quoted text
> ll /proc/sys/fs/xfs
... -rw-r--r-- 1 0 Oct 5 21:35 inherit_noatime -rw-r--r-- 1 0 Oct 5 21:35 inherit_nodefrag -rw-r--r-- 1 0 Oct 5 21:35 inherit_nodump -rw-r--r-- 1 0 Oct 5 21:35 inherit_nosymlinks -rw-r--r-- 1 0 Oct 5 21:35 inherit_sync -rw-r--r-- 1 0 Oct 5 21:35 irix_sgid_inherit -rw-r--r-- 1 0 Oct 5 21:35 irix_symlink_mode ... (specifically irix_sgid_inherit) As you can see, there are ways to change other behaviors to inherit parent settings or not. I think acls are on by default on most file systems EXCEPT with ext[34] they are a mount option. But I don't know of any OS-independent nor FS independent way to detect them. Some versions of 'cp' copy them, as well as some versions of tar and rsync. Show quoted text
> I'll leave this open for now until our next release when we'll move this particular test to the author test suite, mitigating any possible issues from leaving it in the mix. > > Thanks, > Chase > >
Download (untitled) / with headers
text/plain 2.3k
On Fri, 5 Oct 2018 02:01:13 GMT, CAPOEIRAB wrote: Show quoted text
> I'm not sure that I see a way to accurately test the file permissions > unless I move this test to the xt/author section so that it's not > tested for installs on servers where we can't guarantee there aren't > ACLs in place. > > Do you have any other suggestions?
Checking for ACLs in a portable fashion is hard, all right. But that doesn’t mean you can’t check whether your test can possibly test something useful, no? Time for some testing 101. What do you do when you are fixing a bug, to be sure that you fixed it? First you write a test that fails. Then you fix the bug. Then you check that the test passes. Why? Because if you fix the bug first and only then write a test, then you don’t know for sure that that particular test would have failed before the bugfix. And so when it passes, you don’t actually know for sure that *the bugfix* is the reason why the test passes. And so you don’t know for sure what your bugfix actually did. Well… here you have a test that only checks for the outcome you expect. But there is no check that umask actually works. Therefore when the test fails, you don’t actually know that this means the code it tests isn’t working. It might have failed because umask failed instead of because your code failed. So… what *do* you do here to check that? You set the umask to 0, create a file, and test whether getmode($file)==0666. If that’s the case, then umask is doing what you thought it does. If not, you have to skip the tests, because something is interfering with your base assumptions about umask. (Further reading: look up “null hypothesis”.) (Note too that you MUST NOT use write_file for the file creation here, because that is the code you want to test in the first place, and you want to test the null hypothesis outside of any possible interference from that code.) So, putting it all together, what does that look like in practice? In copy-pastable terms – you move the `plan tests => 2` line down to the `umask 027` line and put the following block above that: umask 0 ; unlink $file ; { open my $fh, '>', $file or die $! } my $u0_mode = getmode( $file ) ; unlink $file ; plan skip_all => 'umask 0 does not yield 0666 permissions' if $u0_mode != 0666 ; Linda: can you verify that doing this makes the test skip without failing?
Subject: Re: [rt.cpan.org #92401] test "t/perms.t" is invalid; umask test broken w/ACLs
Date: Sun, 07 Oct 2018 12:44:20 -0700
To: bug-File-Slurp [...] rt.cpan.org
From: "L. A. Walsh" <pause [...] tlinx.org>
Download (untitled) / with headers
text/plain 1.8k
On 10/6/2018 4:25 PM, Aristotle Pagaltzis via RT wrote: Show quoted text
> So… what *do* you do here to check that? You set the umask to 0, create a file, and test whether getmode($file)==0666. If that’s the case, then umask is doing what you thought it does. If not, you have to skip the tests, because something is interfering with your base assumptions about umask. >
--- Not necessarily. If you run that test as I did on a local file, whether the umask was set to '0000' or 0777, the permissions on the file were 0664 in both cases. By changing the acls I'm pretty sure I can come up with it returning 0666 in both 'file' cases. For a directory, I got 2775 with umask set to 0000 and 0777. umasks are also affected by setgid bits on directories. Show quoted text
> I.e. I don't think you test setting > 'umask'only tests umask because you have different perms based on what user you are running and the object you are checking. >
--- Am pretty sure you are not doing that. If you just want to test umask, you could do something like: Show quoted text
> umask 000 > if [[ $(awk 'BEGIN {IGNORECASE=1} /umask/{print $2}' /proc/$$/status)
== 0000 ]]; then echo ok; fi Show quoted text
> (Further reading: look up “null hypothesis”.) > > (Note too that you MUST NOT use write_file for the file creation here, because that is the code you want to test in the first place, and you want to test the null hypothesis outside of any possible interference from that code.) >
--- True, but if you feel a need to test umask, you need to do it without using a file system as not all of them support simple unix mode bits. Show quoted text
> > Linda: can you verify that doing this makes the test skip without failing? >
Sorry, but I can make it fail or pass. See above for testing functionality of umask (which sets a mask in the current process). As for testing what it does on all file systems? *cough*. Am I missing something else (again?)? :-) -l
Download (untitled) / with headers
text/plain 1.8k
On Sun, 7 Oct 2018 19:44:36 GMT, LAWALSH wrote: Show quoted text
> If you run that test as I did on a local file, whether > the umask was set to '0000' or 0777, the permissions on the file were > 0664 in both cases. By changing the acls I'm pretty sure I can come > up with it returning 0666 in both 'file' cases. For a directory, I got > 2775 with umask set to 0000 and 0777.
That doesn’t matter. The question is whether you can set ACLs in such a way that umask 0 produces 0666 *but* umask 027 does *not* produce 0640. Show quoted text
> umasks are also affected by setgid bits on directories.
Those *definitely* don’t matter since they just end up affecting the regular Unix permission bits which compose cleanly with umask. So in a directory with a relevant setgid, the test will be skipped, because umask 0 will not produce a 0666 file. Show quoted text
> > Linda: can you verify that doing this makes the test skip without > > failing? > >
> Sorry, but I can make it fail or pass.
Eh? How can a test with two possible outcomes neither fail nor pass…? Show quoted text
> Am I missing something else (again?)? :-)
Depends on how freely you can screw up umask with ACLs, I guess. I suppose I didn’t full apply my own logic – testing what umask 0 does and then expecting that other umasks will have corresponding effects is not a null hypothesis for what is being tested. Instead, the first test could check that umask 027 yields 0640 before testing that write_file under umask 027 also produces an 0640 file, and the second test could check that an 0777 mode yields an 0750 file before testing that that’s what write_file does. (That would also permit running the test on Windows, rather than skipping it on general principle, where it would then just end up skipping both tests individually. Basically trading agent sniffing for feature detection, in webdev terms.) Will suggest a patch later.
Download (untitled) / with headers
text/plain 171b
This was resolved via https://github.com/perhunter/slurp/commit/3e93bf741e0f16698821d594a09b4983a7536fd8 and will be in a release that will be cut shortly. Thanks, Chase


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.