Improved the site module's permission handling while writing .python_history

[opensource-assist]

BPO	39468
Nosy	@stevendaprano, @eryksun, @opensource-assist
PRs	bpo-39468: Improve the site module's error handling while writing .python_history #18299 bpo-39468: better .python_history write permission handling #18210

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-01-27.19:46:52.958>
labels = ['type-feature', 'library', '3.9']
title = "Improved the site module's permission handling while writing .python_history"
updated_at = <Date 2021-05-03.21:29:17.689>
user = 'https://github.com/opensource-assist'

bugs.python.org fields:

activity = <Date 2021-05-03.21:29:17.689>
actor = 'opensource-assist'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2020-01-27.19:46:52.958>
creator = 'opensource-assist'
dependencies = []
files = []
hgrepos = []
issue_num = 39468
keywords = ['patch']
message_count = 5.0
messages = ['360790', '360791', '360794', '360871', '360889']
nosy_count = 4.0
nosy_names = ['steven.daprano', 'SilentGhost', 'eryksun', 'opensource-assist']
pr_nums = ['18299', '18210']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue39468'
versions = ['Python 3.9']

[opensource-assist]

On a typical Linux system, if you run 'chattr +i /home/user/.python_history', and then run python, then exit, the following error message will be printed out:
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site.py", line 446, in write_history
    readline.write_history_file(history)
OSError: [Errno -1] Unknown error -1

With a simple improvement, the site module can check and suggest the user to run 'chattr -i' on the .python_history file.

Additionaly, I don't know if it's a good idea to automatically run 'chattr -i' in such a situation or not.

[opensource-assist]

https://github.com/opensource-assist/cpython/blob/opensource-assist-patch-sitepy-1/Lib/site.py

[SilentGhost]

1. Your PR contains unrelated changes
2. Please test your code: submitting syntactically incorrect code just wastes everyone's time

[stevendaprano]

Reporting a better error message than just "Unknown error -1" is a good idea. Stating "an error occurred..." is hardly any better.

The correct error is, I think, "permission denied". Trying to diagnose the *specific* issue (read-only file? file owned by another user? read-only file system? samba permissions? etc) is probably a waste of time. The human reading the error can do that.

Having Python automatically run chattr -i is a bad design. If I've made the history file immutable, it is because I want it to be immutable, not because I want random applications to try to sneakily make it mutable again. Python's role in this should end when it reports that it doesn't have write permission to the file.

[eryksun]

This issue is due to a bug in GNU Readline (actually GNU History). It's documented that write_history [1] returns an errno value. But the internal history_do_write [2] function in this case returns the value from a rename() system call, via the value from histfile_restore [3]. rename() returns -1 on failure, which is being mishandled here as an errno value.

Actually, write permission to the original history file isn't required. GNU History writes to a temp file and then replaces the original via rename(). Normally this just requires write access to the directory. But if the directory or target file has the immutable file attribute set, then rename() fails with errno set to EPERM (at least in Linux; maybe it's a different error in BSD or macOS).

If not for the GNU History bug, this failed call would raise a PermissionError, which is already handled. Maybe it could also write an error message to stderr that write_history failed. For example:

            def write_history():
                try:
                    readline.write_history_file(history)
                except OSError as e:
                    # bpo-19891: home directory does not exist or is not
                    # writable
                    if not (isinstance(e, (FileNotFoundError, PermissionError))
                    # bpo-39468: GNU History may return -1 as an errno value
                            or e.errno == -1):
                        raise
                    print('write_history failed: {!r}'.format(history),
                        file=sys.stderr)

I agree with Steven that the handler should not presume to modify file permissions or attributes.

[1] https://tiswww.case.edu/php/chet/readline/history.html#IDX29
[2] http://git.savannah.gnu.org/cgit/readline.git/tree/histfile.c#n630
[3] http://git.savannah.gnu.org/cgit/readline.git/tree/histfile.c#n458