Issue 19037: mailbox module modifies maildir file times after moving from tmp directory (original) (raw)

Created on 2013-09-16 18:58 by janzert, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Messages (7)
msg197930 - (view)	Author: janzert (janzert) *	Date: 2013-09-16 18:58
The Maildir.add and Maildir.__setitem__ methods in the mailbox module attempts to change the file mtime after moving the file into the new directory. This allows a race condition since other programs are can move or otherwise modify the file as soon as it is placed in the new directory. If the file is moved from the new directory before the mtime is set this results in an unexpected OSError exception. The fix seems to simply be setting the file mtime before moving the file into its final position.
msg197933 - (view)	Author: R. David Murray (r.david.murray) *	Date: 2013-09-16 19:10
That sounds reasonable. Would you be interested in trying your hand at a patch, ideally with a test?
msg197935 - (view)	Author: janzert (janzert) *	Date: 2013-09-16 19:25
I can certainly write a patch if wanted. It should be simply moving and modifying two lines in each of the two methods. My understanding is that it should be against 2.7 so it can be applied there first then merged forward? Unfortunately while I can consistently reproduce the problem, with a script using pyinotify, I'm pretty lost how to write a test that would catch this.
msg197941 - (view)	Author: R. David Murray (r.david.murray) *	Date: 2013-09-16 19:51
Hopefully a 2.7 patch would also apply to 3.3, so yes, start there. For the test, I was thinking that in 3.3+ we could use mock to introduce a delay. But looking at the code again it isn't obvious that there is a meaningful way to do it that is worth the effort required. So let's forget about the test for this one...a comment about why the time set is done before the move would be worthwhile, though.
msg198018 - (view)	Author: R. David Murray (r.david.murray) *	Date: 2013-09-18 12:34
Janzert: Thanks for the patch. A contributor agreement is not needed for this patch, since it just moves code around, but you might want to submit one in case you make any other contributions. Also let us know what name to use in the Misc/ACKS file.
msg198019 - (view)	Author: Roundup Robot (python-dev)	Date: 2013-09-18 12:39
New changeset 68e5e416e8af by R David Murray in branch '3.3': #19037: adjust file times *before* moving maildir files into place. http://hg.python.org/cpython/rev/68e5e416e8af New changeset 041caa64486b by R David Murray in branch '2.7': #19037: adjust file times *before* moving maildir files into place. http://hg.python.org/cpython/rev/041caa64486b New changeset 261910257af6 by R David Murray in branch 'default': Merge #19037: adjust file times *before* moving maildir files into place. http://hg.python.org/cpython/rev/261910257af6
msg198026 - (view)	Author: janzert (janzert) *	Date: 2013-09-18 16:16
Thanks for committing the fix. Figured I should finally get it done and signed the online CLA when I submitted this issue. I assume it just takes a while for someone to go through and apply the appropriate flag? This change seems rather insubstantial for an ACKS line so I certainly don't mind waiting. Since a certain Birkenfeld became Brandl I know names are a bit sensitive for some python core devs. So while my personal preference would go to Janzert* the one that fits Python custom is Brian Haskin. * Janzert is the most widely used and distinct identifier for my open source and online activity.