mailbox.mbox.flush() loses mailbox owner

[softins]

In the flush() method for mbox mailboxes, after writing the new file, it correctly copies the file modes from the old file to the new one:

cpython/Lib/mailbox.py

Lines 753 to 755 in 027fa2e

	# Make sure the new file's mode is the same as the old file's
	mode = os.stat(self._path).st_mode
	os.chmod(new_file.name, mode)

However, it doesn't copy the user.group ownership of the old file to the new one, and I think it should.

I implemented a python program to modify a mailbox, in this case to delete old messages from a spam folder. The program runs as root, so that it can operate on folders belonging to different users without having to suid to each user. I found that on flushing after any changes, the mailbox was owned by root.root instead of by its original owner.

Suggest the following code instead:

        # Make sure the new file's mode and owner are the same as the old file's
        info = os.stat(self._path)
        os.chmod(new_file.name, info.st_mode)
        try:
                os.chown(new_file.name, info.st_uid, info.st_gid)
        except OSError:
                pass

Linked PRs

• gh-117467: Add preserving of mailbox owner on flush #117510
• [3.12] gh-117467: Add preserving of mailbox owner on flush (GH-117510) #117537

[serhiy-storchaka]

The suggestion LGTM. Do you mind to create a PR, @softins?

[softins]

The suggestion LGTM. Do you mind to create a PR, @softins?

Sure, will do. I think I will actually change except OSError: to a general except:, in case of other exceptions such as os.chown() not being present on a particular platform.

[serhiy-storchaka]

Silently ignoring arbitrary exceptions is never a good idea. KeyboardInterrupt, MemoryError and RecursionError can be raised by virtually any code.

It is better to make the exception check more specific, for example: (PermissionError, AttributeError).