Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#3215 defect closed fixed (fixed)

Test failure in twisted.test.test_logfile.LogFileTestCase.test_noPermission

Reported by: TimAllen Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone Branch: branches/nfs-log-3215
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

I checked out revision 23447 from the svn.twistedmatrix.com/svn/Twisted/trunk on my Fedora Core 6 workstation (using the stock Python 2.4.4; I think FC6 also has SELinux enabled by default). I ran the Twisted unit tests as follows:

PYTHONPATH=$PWD bin/trial twisted

After some false-starts (due, I think, to bug 3111 - after I installed pyOpenSSL a whole bunch of ERRORs disappeared), I was left with a single test-failure: twisted.test.test_logfile.LogFileTestCase.test_noPermission:

===============================================================================
[ERROR]: twisted.test.test_logfile.LogFileTestCase.test_noPermission

Traceback (most recent call last):
  File "/mnt/aquaria/nobackup/home/tim/tmp/Twisted/twisted/test/test_logfile.py", line 168, in test_noPermission
    self.assertEquals(f.read(), "abcdef")
exceptions.IOError: [Errno 5] Input/output error
-------------------------------------------------------------------------------

I'm not exactly sure why this test fails, but my suspicion is that when the test calls f.read(), Python checks to see if the file is still readable. However, earlier in the test we've chmod'd the directory to 0444, so Python is prevented from checking the permissions and bails out.

At any rate, changing the chmod to 0555 (including the permission-reading permission) makes the test pass:

diff --git a/twisted/test/test_logfile.py b/twisted/test/test_logfile.py
index a4c0b0f..887b549 100644
--- a/twisted/test/test_logfile.py
+++ b/twisted/test/test_logfile.py
@@ -143,7 +143,7 @@ class LogFileTestCase(unittest.TestCase):
         log.write("abc")
 
         # change permissions so rotation would fail
-        os.chmod(self.dir, 0444)
+        os.chmod(self.dir, 0555)
 
         # if this succeeds, chmod doesn't restrict us, so we can't
         # do the test

Change History (13)

comment:1 Changed 11 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

Does SELinux impose this restriction? Normally, you do not need read permission on the filename which was used to get a file descriptor in order to read from that file descriptor:

>>> f = file('foo', 'w')
>>> f.write('hello')
>>> f.close()
>>> f = file('foo', 'r')
>>> import os
>>> os.chmod('foo', 0)
>>> f.read()
'hello'
>>>

comment:2 Changed 11 years ago by Jean-Paul Calderone

Ah, you were talking about the directory, which my previous example didn't cover. The behavior is the same, though:

>>> import os
>>> os.mkdir('foo')
>>> f = file('foo/bar', 'w')
>>> f.write('hello')
>>> f.close()
>>> f = file('foo/bar', 'r')
>>> os.chmod('foo', 0)
>>> f.read()
'hello'
>>>

comment:3 Changed 11 years ago by TimAllen

The code fragment you wrote in comment 2 works on my system as well, however this code does not:

f = file('foo/bar', 'w+')
f.write('hello')
f.seek(0)
os.chmod('foo', 0)
f.read()
f.close()
print "Test passed"

Actually, even this code works on my system in /tmp, or some other local file-system. It only breaks in my home directory, which is an NFS mount. A system-administrator friend of mine confirmed that POSIX checks permissions only when you open a file, and NFS checks every time the file is accessed.

comment:4 Changed 11 years ago by TimAllen

Keywords: review added
Owner: Glyph deleted

I believe that the patch I posted in the original comment is still the best approach; requesting review.

comment:5 Changed 11 years ago by Jean-Paul Calderone

Thanks for following up on this, Tim. I'm a bit curious about some further behavior, though. What is the behavior of attempting a write after using chmod to remove all permissions from the directory? If writes don't work in this scenario, then the behavior this test covers doesn't really make sense, since not only will rotation fail, but writing out log messages themselves will be impossible.

comment:6 Changed 11 years ago by TimAllen

Ahh, good idea. I experimented a little with NFS permissions, and here's what I found.

Here's the simple, obvious test:

import os
os.mkdir("foo")
f = open("foo/bar", 'w+')
f.write("test1\n")
os.chmod("foo", 0444)
f.write("test2\n")
f.close()

It crashes with "permission denied" at f.close()... at which point the file foo/bar is still empty! Not only did the second write not succeed, the first write didn't either.

I decided to blame stdio's buffering, and tried again with it disabled:

import os
os.mkdir("foo")
f = open("foo/bar", 'w+', 0)
f.write("test1\n")
os.chmod("foo", 0444)
f.write("test2\n")
f.close()

Exactly the same behaviour - the file still empty.

I pulled out the big gun, os.fsync():

import os
os.mkdir("foo")
f = open("foo/bar", 'w+', 0)
f.write("test1\n")
os.fsync(f)
os.chmod("foo", 0444)
f.write("test2\n")
os.fsync(f)
f.close()

Now we're getting somewhere - the script still hits "permission denied" at f.close(), but at least now the file foo/bar contains "test1\n" as it should.

Then I tried changing the permissions of the directory, as suggested in the patch in my original comment:

import os
os.mkdir("foo")
f = open("foo/bar", 'w+', 0)
f.write("test1\n")
os.fsync(f)
os.chmod("foo", 0555)
f.write("test2\n")
os.fsync(f)
f.close()

The script runs to completion, the file contains "test1\ntest2\n".

If I get rid of all the flushing and synching:

import os
os.mkdir("foo")
f = open("foo/bar", 'w+')
f.write("test1\n")
os.chmod("foo", 0555)
f.write("test2\n")
f.close()

...the script still works as expected.

If I add a test for log-renaming:

import os
os.mkdir("foo")
f = open("foo/bar", 'w+')
f.write("test1\n")
os.chmod("foo", 0555)
try:
	os.rename("foo/bar", "foo/bar.1")
except OSError:
	print "log rotation is prevented,"
f.write("test2\n")
f.close()

...the script prints the "log rotation is prevented" message, it still exits cleanly, and the file still contains "test1\ntest2\n".

In summary, using 0555 as the directory permissions seems to allow everything we want and deny everything we don't, even on NFS-mounted filesystems, with no further flushing or synching requirements.

comment:7 Changed 11 years ago by Glyph

author: glyph
Branch: branches/nfs-log-3215

(In [24079]) Branching to 'nfs-log-3215'

comment:8 Changed 11 years ago by Glyph

I'm running the tests in

http://buildbot.twistedmatrix.com/builders/winxp32-py2.5-select/builds/425

to make sure that this doesn't have any unfortunate interactions with Windows's peculiar interpretation of permissions.

If that succeeds, then the branch should be merged. However, if you really care about Twisted supporting logging to NFS, there are probably a bunch of other tests that have to be written, as well as a buildbot that needs to run to make sure that none of our other file operations are non-NFS-posix-semantics-dependent.

comment:9 Changed 11 years ago by Glyph

Owner: set to Glyph
Status: newassigned

comment:10 in reply to:  8 Changed 11 years ago by TimAllen

Replying to glyph:

If that succeeds, then the branch should be merged. However, if you really care about Twisted supporting logging to NFS, there are probably a bunch of other tests that have to be written, as well as a buildbot that needs to run to make sure that none of our other file operations are non-NFS-posix-semantics-dependent.

I don't intend to log to NFS mounts in production (I hope I never have to use any NFS in production), so I don't care enough to set up a buildbot, or finding and writing tests for all the other possible mismatches between NFS and POSIX semantics. However I still think this test ought to be fixed, just so that new users won't get discouraged when they install Twisted for the first time and discover tests failing.

comment:11 Changed 11 years ago by therve

Resolution: fixed
Status: assignedclosed

(In [24125]) Merge nfs-log-3215

Author: TimAllen Reviewers: glyph, therve Fixes #3215

Make twisted.test.test_logfile.LogFileTestCase.test_noPermission pass on NFS.

comment:12 Changed 11 years ago by therve

Keywords: review removed

comment:13 Changed 9 years ago by <automation>

Owner: Glyph deleted
Note: See TracTickets for help on using tickets.