msanger / flyback

Automatically exported from code.google.com/p/flyback
0 stars 0 forks source link

Race condition in install_crontab #15

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
There is a race-condition in install_crontab, see my comments:

    def install_crontab(self, c):
        existing_crons = []

        stdin, stdout = os.popen4('crontab -l')
        # we do now have the crontab at timestamp T
        for line in stdout:
            if line.startswith('no crontab for'): continue
            if line.endswith('#flyback\n'): continue
            existing_crons.append(line)
        if c:
            existing_crons.append(c + ' python '+ os.getcwd() +'/flyback.py
--backup #flyback\n')
        stdin.close()
        stdout.close()
        # we have parsed it and needed n time for that, so now we have T+n
        # during the time needed for parsing, the user has added another
crontab entry

        f = open('/tmp/flyback_tmp_cron', 'w')
        f.writelines( existing_crons )
        f.close()
        os.system('crontab /tmp/flyback_tmp_cron')
        # we are writing the crontab as of timestamp T plus our small change
        # deleting the change the user did somewhere between T and T+n :(

Hope it's clear ;)
If not, bomb me via email

Original issue reported on code.google.com by zhen...@gmail.com on 9 Nov 2007 at 8:08

GoogleCodeExporter commented 9 years ago
yeah, but isn't this true of all things that install via crontab?  there is no
"crontab --add", only read/delete/replace.

Original comment by pub...@kered.org on 9 Nov 2007 at 3:37

GoogleCodeExporter commented 9 years ago
closing since this is an issue w/ crontab.
(comment as to why i'm wrong and i'll reopen)

Original comment by pub...@kered.org on 10 Nov 2007 at 8:32

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
From the crontab man page: "The specified editor must edit the file in place; 
any 
editor that unlinks the file and recreates it cannot be used."

It sounds like the recommended approach is to run "crontab -e" and then edit 
the 
file in place, with appropriate locking to avoid the race condition.

Original comment by staz6...@gmail.com on 12 Nov 2007 at 1:24

GoogleCodeExporter commented 9 years ago
Well, at least here (with vixie cron on Debian Sid) that does not solve 
anything.
Open 2 terminals, run crontab -e in each, and have fun with overwriting :(
Additionally, my "man crontab" does not show your line :(

Original comment by zhen...@gmail.com on 12 Nov 2007 at 1:42

GoogleCodeExporter commented 9 years ago
yeah, i can confirm comment #5 here on gusty.

fortunately while usually i'm a real stickler for multithreaded correctness, i 
think
this is unlikely to cause any real world problems.  we only open it for a 
fraction of
a second, and therefore are likely to lose any races that may occur.  and since 
we
write it on every prefs save, it'll just get updated in the next one.

additionally, my crontab specifically mentions using sed to edit it this way, 
which
has exactly the same theoretical problem.

Original comment by pub...@kered.org on 12 Nov 2007 at 6:46