classification
Title: mailbox module modifies maildir file times after moving from tmp directory
Type: behavior Stage: resolved
Components: email, Library (Lib) Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: barry, janzert, petri.lehtinen, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2013-09-16 18:58 by janzert, last changed 2013-09-18 16:16 by janzert. This issue is now closed.

Files
File name Uploaded Description Edit
mailbox.patch janzert, 2013-09-16 20:26 Patch to fix race condition in maildir support review
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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.
History
Date User Action Args
2013-09-18 16:16:19janzertsetmessages: + msg198026
2013-09-18 12:39:59r.david.murraysetstatus: open -> closed
resolution: fixed
stage: resolved
2013-09-18 12:39:32python-devsetnosy: + python-dev
messages: + msg198019
2013-09-18 12:34:23r.david.murraysetmessages: + msg198018
2013-09-16 20:26:11janzertsetfiles: + mailbox.patch
keywords: + patch
2013-09-16 19:51:35r.david.murraysetmessages: + msg197941
2013-09-16 19:25:44janzertsetmessages: + msg197935
2013-09-16 19:10:11r.david.murraysetnosy: + barry, r.david.murray
messages: + msg197933
components: + email
2013-09-16 18:58:36janzertcreate