On 05/17/12 21:27, James Cammarata wrote:
> On Thu, May 17, 2012 at 3:25 AM, Jun'ichi Nomura <j-nom...@ce.jp.nec.com>
> wrote:
>> while using cobbler, I've hit sometimes a strange problem,
>> that cobblerd starts creating files with wrong permissions like:
>>
>> --w------- 2 root root 11405684 May 16 14:57 initrd.img
>>
>> Basically, regular files become "--w-------" and directories become
>> "d-w---x---".
>>
>> Once happen, it continues to generate files with such permissions
>> until restarting the daemon.
>>
>> Investigating with 'crash' command, I found cobblerd had wrong umask
>> when the problem happened:
>>
>> crash> ps cobblerd
>> PID PPID CPU TASK ST %MEM VSZ RSS COMM
>> 4693 1 0 ffff810027fbc100 IN 0.7 375980 31688 cobblerd
>> crash> struct task_struct.fs 0xffff810027fbc100
>> fs = 0xffff810035f6fb00,
>> crash> struct fs_struct.umask 0xffff810035f6fb00
>> umask = 0x177,
>>
>> Normally it has the following umask:
>>
>> crash> struct fs_struct.umask 0xffff810035f6fb80
>> umask = 0x12,
>>
>> The wrong umask explains why the generated files/directories have
>> such a strange permissions.
>>
>> I found the following code in api.py and serializer.py are suspicious:
>>
>> old = os.umask(0x777)
>> fd = open("/var/lib/cobbler/.mtime","w")
>> fd.write("%f" % time.time())
>> fd.close()
>> os.umask(old)
>>
>> Firstly, "0x777" seems strange. I guess it should be octal "777".
>> It seems python ignores upper bits and set "0x177" when you do
>> os.umask(0x777).
>>
>> Secondly, if the above code is not serialized each other,
>> it would end up with wrong umask as follows:
>>
>> Context A Context B
>> ------------------------------------------
>> old = umask(0x777)
>> old = umask(0x777)
>> [old is 0x777]
>> umask(old)
>> umask(old)
>>
>> Any comments?
>
> Very interesting, this was reported in issue #117
> (https://github.com/cobbler/cobbler/issues/117) though it hadn't gone
> anywhere. I had originally thought cobbler was perhaps chmod'ing files
> but did not look at it potentially changing umask like this. Overall,
> it looks like it's being done wrong consistently through the code:
>
> cobbler/cobblerd.py: um = os.umask(int('0027',16))
> cobbler/cobblerd.py: os.umask(um)
> cobbler/api.py: old = os.umask(0x777)
> cobbler/api.py: os.umask(old)
> cobbler/serializer.py: old = os.umask(0x777)
> cobbler/serializer.py: os.umask(old)
> scripts/cobblerd: os.umask(022)
>
> That first one should probably be int('0027',8) and the last one
> should probably be 0022 (that may be ok as-is).
>
> Would it be possible for you to fork the code via github and correct
> the above issues and test this? I have been unable to replicate the
> issue locally so if you can reliably reproduce this you'll be better
> able to test this out.
Thank you for the comments.
Unfortunately, I've seen it only a few times in a year...
I wonder whether those functions using umask are called
from threads (or signal handlers).
If they do, it would certainly cause such a problem
in the same way as the following reproducer.
---------------------------------------------------------
#!/usr/bin/python
import os
import time
import thread
import sys
origmask = 0002
badmask = 0x777
def loop(file,mask):
while True:
old = os.umask(mask)
if (old != origmask):
print "wrong umask %o vs expected %o" % (old,origmask)
sys.exit(1)
fd = open(file,"w")
fd.write("%f" % time.time())
fd.close()
os.umask(old)
os.unlink(file)
os.umask(origmask)
thread.start_new_thread(loop, ("test1", badmask))
loop("test2",origmask)
---------------------------------------------------------
And I think the solution would be something like the attached patch,
using os.open() with mode parameter to avoid os.umask.
Thanks,
--
Jun'ichi Nomura, NEC Corporation
diff --git a/cobbler/api.py b/cobbler/api.py
index a5432a5..92a76ff 100644
--- a/cobbler/api.py
+++ b/cobbler/api.py
@@ -181,11 +181,9 @@ class BootAPI:
API instance, regardless of the serializer type.
"""
if not os.path.exists("/var/lib/cobbler/.mtime"):
- old = os.umask(0x777)
- fd = open("/var/lib/cobbler/.mtime","w")
- fd.write("0")
- fd.close()
- os.umask(old)
+ fd = os.open("/var/lib/cobbler/.mtime", os.O_CREAT|os.O_RDWR, 0200)
+ os.write(fd, "0")
+ os.close(fd)
return 0
fd = open("/var/lib/cobbler/.mtime")
data = fd.read().strip()
diff --git a/cobbler/serializer.py b/cobbler/serializer.py
index 0f67dbe..f78c994 100644
--- a/cobbler/serializer.py
+++ b/cobbler/serializer.py
@@ -64,11 +64,9 @@ def __release_lock(with_changes=False):
# this file is used to know when the last config change
# was made -- allowing the API to work more smoothly without
# a lot of unneccessary reloads.
- old = os.umask(0x777)
- fd = open("/var/lib/cobbler/.mtime","w")
- fd.write("%f" % time.time())
- fd.close()
- os.umask(old)
+ fd = os.open("/var/lib/cobbler/.mtime", os.O_CREAT|os.O_RDWR, 0200)
+ os.write(fd, "%f" % time.time())
+ os.close(fd)
if LOCK_ENABLED:
LOCK_HANDLE = open("/var/lib/cobbler/lock","r")
fcntl.flock(LOCK_HANDLE.fileno(), fcntl.LOCK_UN)
_______________________________________________
cobbler-devel mailing list
cobbler-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/cobbler-devel
