This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: logging TimedRotatingFileHandler must not rename devices like /dev/null
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: miss-islington, serhiy.storchaka, vinay.sajip, vstinner
Priority: normal Keywords: patch

Created on 2021-10-07 09:47 by vstinner, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 28822 merged vinay.sajip, 2021-10-08 16:11
PR 28864 merged serhiy.storchaka, 2021-10-11 07:58
PR 28865 closed vstinner, 2021-10-11 07:58
PR 28866 merged miss-islington, 2021-10-11 08:56
PR 28867 merged miss-islington, 2021-10-11 08:56
PR 28872 merged miss-islington, 2021-10-11 10:01
PR 28873 merged miss-islington, 2021-10-11 10:02
Messages (15)
msg403371 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-10-07 09:47
One way to disable logging in a configuration file is to use /dev/null as the filename for logs. It is fine to use that with FileHandler.

The problem is when TimedRotatingFileHandler is used. When Python decides to rotate the "file", it calls os.rename() on /dev/null *device* and create a new /dev/null *file* which will be quickly filled by all applications on the system writing into /dev/null.

The problem is only possible if the process is allowed to rename the device /dev/null. For example, a regular user gets a permission error.

I see different options:

* TimedRotatingFileHandler should disable rotation if it detects that the file is a device (not a regular file)
* logging should switch to NullHandler if filename is equal to os.path.devnull.

I guess that the problem is the same if the filename points to a socket or a named pipe.


RotatingFileHandler may also be affected, I didn't check.


... I'm quite sure that I had the same issue with Twisted years ago, but I don't recall the details :-) Twisted didn't use the logging module if I recall correctly.


RHEL downstream issue: https://bugzilla.redhat.com/show_bug.cgi?id=2009200
msg403433 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2021-10-07 19:08
RotatingFileHandler is also affected by the same issue.

Both RotatingFileHandler and TimedRotatingFileHandler have a shouldRollover method which, if it returns False, should prevent rollover.

I would think that putting something like

    if not os.path.isfile(self.baseFilename):
        return False

at the top of each of these methods should avoid rolling over any thing that isn't a regular file - devices, pipes etc. Would you agree? Can you see any drawbacks?

I'm not sure this would cover sockets, but there are separate handlers for using sockets so there would be no reason for people to use sockets with file handlers. And of course sockets are a completely different sort of beast on Windows.
msg403593 - (view) Author: miss-islington (miss-islington) Date: 2021-10-10 15:15
New changeset 62a667784ba7b84611ebd50fa8a1a464cde32235 by Vinay Sajip in branch 'main':
bpo-45401: Change shouldRollover() methods to only rollover regular f… (GH-28822)
https://github.com/python/cpython/commit/62a667784ba7b84611ebd50fa8a1a464cde32235
msg403604 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-10-10 17:50
Seems it introduced a resource warning in tests.
msg403606 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2021-10-10 20:21
Where did you see the warnings? I didn't spot anything in the GitHub Actions logs.
msg403607 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-10-10 20:28
test_should_not_rollover (test.test_logging.TimedRotatingFileHandlerTest) ... /home/serhiy/py/cpython/Lib/unittest/case.py:547: ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='a' encoding='utf-8'>
  if method() is not None:
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
msg403608 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-10-10 20:30
With tracemalloc enabled:

$ ./python -X tracemalloc -m test -v test_logging -m test_should_not_rollover
...
test_should_not_rollover (test.test_logging.TimedRotatingFileHandlerTest) ... /home/serhiy/py/cpython/Lib/unittest/case.py:547: ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='a' encoding='utf-8'>
  if method() is not None:
Object allocated at (most recent call last):
  File "/home/serhiy/py/cpython/Lib/logging/__init__.py", lineno 1205
    return open_func(self.baseFilename, self.mode,
ok
msg403627 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-10-11 07:58
Vinay: Oh, thanks for the fix! Using os.path.isfile() is a reasonable fix to detect if the file is a device.
msg403629 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-10-11 08:02
> $ ./python -X tracemalloc -m test -v test_logging -m test_should_not_rollover

FYI you can use -X tracemalloc=10 to get a more complete traceback.
msg403636 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-10-11 08:54
New changeset 15188b115a2da815556053372c912a81a74be43b by Serhiy Storchaka in branch 'main':
bpo-45401: Fix a resource warning in test_logging (GH-28864)
https://github.com/python/cpython/commit/15188b115a2da815556053372c912a81a74be43b
msg403643 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-10-11 10:01
New changeset ac421c348bf422f9a0d85fe0a1de3fa3f4886650 by Miss Islington (bot) in branch '3.9':
bpo-45401: Change shouldRollover() methods to only rollover regular f… (GH-28822) (#28866)
https://github.com/python/cpython/commit/ac421c348bf422f9a0d85fe0a1de3fa3f4886650
msg403644 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-10-11 10:01
New changeset 5aca34f17c4baf8e4882a7e8a827cff06ac6ef25 by Miss Islington (bot) in branch '3.10':
bpo-45401: Change shouldRollover() methods to only rollover regular f… (GH-28822) (#28867)
https://github.com/python/cpython/commit/5aca34f17c4baf8e4882a7e8a827cff06ac6ef25
msg403711 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-10-12 07:18
New changeset faa87f7f3b60f79b9018aaef0efa5e00d82b817b by Miss Islington (bot) in branch '3.9':
bpo-45401: Fix a resource warning in test_logging (GH-28864) (GH-28873)
https://github.com/python/cpython/commit/faa87f7f3b60f79b9018aaef0efa5e00d82b817b
msg403712 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-10-12 07:19
New changeset 47a50fe16f9f074daaaa3e6aa85e76502955ed40 by Miss Islington (bot) in branch '3.10':
bpo-45401: Fix a resource warning in test_logging (GH-28864) (GH-28872)
https://github.com/python/cpython/commit/47a50fe16f9f074daaaa3e6aa85e76502955ed40
msg403717 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-10-12 08:42
Thanks for the fixes Vinay and Serhiy!
History
Date User Action Args
2022-04-11 14:59:50adminsetgithub: 89564
2021-10-12 08:42:08vstinnersetstatus: open -> closed

stage: patch review -> resolved
messages: + msg403717
versions: + Python 3.9, Python 3.10
2021-10-12 07:19:13serhiy.storchakasetmessages: + msg403712
2021-10-12 07:18:46serhiy.storchakasetmessages: + msg403711
2021-10-11 10:02:06miss-islingtonsetpull_requests: + pull_request27171
2021-10-11 10:01:51miss-islingtonsetpull_requests: + pull_request27170
2021-10-11 10:01:17vstinnersetmessages: + msg403644
2021-10-11 10:01:17vstinnersetmessages: + msg403643
2021-10-11 08:56:09miss-islingtonsetpull_requests: + pull_request27168
2021-10-11 08:56:04miss-islingtonsetpull_requests: + pull_request27167
2021-10-11 08:54:48serhiy.storchakasetmessages: + msg403636
2021-10-11 08:02:39vstinnersetmessages: + msg403629
2021-10-11 07:58:59vstinnersetmessages: + msg403627
2021-10-11 07:58:40vstinnersetpull_requests: + pull_request27166
2021-10-11 07:58:11serhiy.storchakasetstage: resolved -> patch review
pull_requests: + pull_request27165
2021-10-10 20:30:20serhiy.storchakasetmessages: + msg403608
2021-10-10 20:28:15serhiy.storchakasetmessages: + msg403607
2021-10-10 20:21:24vinay.sajipsetmessages: + msg403606
2021-10-10 20:16:29serhiy.storchakasetstatus: closed -> open
2021-10-10 17:50:55serhiy.storchakasetmessages: + msg403604
2021-10-10 15:23:47vinay.sajipsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-10-10 15:15:28miss-islingtonsetnosy: + miss-islington
messages: + msg403593
2021-10-08 16:11:44vinay.sajipsetkeywords: + patch
stage: patch review
pull_requests: + pull_request27140
2021-10-08 09:11:57serhiy.storchakasetnosy: + serhiy.storchaka
2021-10-07 19:08:47vinay.sajipsetmessages: + msg403433
2021-10-07 09:47:34vstinnercreate