classification
Title: Improved the site module's permission handling while writing .python_history
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: SilentGhost, eryksun, opensource-assist, steven.daprano
Priority: normal Keywords: patch

Created on 2020-01-27 19:46 by opensource-assist, last changed 2021-05-03 21:29 by opensource-assist.

Pull Requests
URL Status Linked Edit
PR 18299 open opensource-assist, 2020-01-31 16:45
PR 18210 closed opensource-assist, 2021-05-03 21:29
Messages (5)
msg360790 - (view) Author: Aurora (opensource-assist) * Date: 2020-01-27 19:46
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.
msg360791 - (view) Author: Aurora (opensource-assist) * Date: 2020-01-27 19:57
https://github.com/opensource-assist/cpython/blob/opensource-assist-patch-sitepy-1/Lib/site.py
msg360794 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2020-01-27 20:09
1. Your PR contains unrelated changes
2. Please test your code: submitting syntactically incorrect code just wastes everyone's time
msg360871 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2020-01-28 13:04
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.
msg360889 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2020-01-28 18:34
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
History
Date User Action Args
2021-05-03 21:29:17opensource-assistsetstatus: pending -> open
pull_requests: + pull_request24543
2020-01-31 16:46:01opensource-assistsetstatus: open -> pending
2020-01-31 16:45:44opensource-assistsettitle: .python_history write permission improvements -> Improved the site module's permission handling while writing .python_history
2020-01-31 16:45:15opensource-assistsetpull_requests: + pull_request17677
2020-01-31 16:45:07opensource-assistsetpull_requests: - pull_request17675
2020-01-31 16:45:00opensource-assistsetpull_requests: - pull_request17674
2020-01-31 16:44:52opensource-assistsetpull_requests: - pull_request17589
2020-01-31 16:44:11opensource-assistsetpull_requests: + pull_request17675
2020-01-31 16:43:26opensource-assistsetpull_requests: + pull_request17674
2020-01-28 18:34:57eryksunsetnosy: + eryksun
messages: + msg360889
2020-01-28 13:04:26steven.dapranosetnosy: + steven.daprano
messages: + msg360871
2020-01-27 20:09:47SilentGhostsetnosy: + SilentGhost
messages: + msg360794
2020-01-27 20:02:02opensource-assistsetpull_requests: + pull_request17589
2020-01-27 20:01:54opensource-assistsetpull_requests: - pull_request17586
2020-01-27 19:59:52opensource-assistsetkeywords: + patch
stage: patch review
pull_requests: + pull_request17586
2020-01-27 19:57:05opensource-assistsetmessages: + msg360791
2020-01-27 19:46:52opensource-assistcreate