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