Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-95041: Fail syslog.syslog in case inner call to syslog.openlog fails #95264

Merged
merged 5 commits into from Jul 26, 2022

Conversation

noamcohen97
Copy link
Contributor

@noamcohen97 noamcohen97 commented Jul 26, 2022

@bedevere-bot
Copy link

bedevere-bot commented Jul 26, 2022

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

bedevere-bot commented Jul 26, 2022

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

bedevere-bot commented Jul 26, 2022

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Modules/syslogmodule.c Outdated Show resolved Hide resolved
Modules/syslogmodule.c Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Jul 26, 2022

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

bedevere-bot commented Jul 26, 2022

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error skip news needs backport to 3.10 needs backport to 3.11 type-crash A hard crash of the interpreter, possibly with a core dump labels Jul 26, 2022
Modules/syslogmodule.c Outdated Show resolved Hide resolved
Modules/syslogmodule.c Outdated Show resolved Hide resolved
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

LGTM. Thanks, Noam!

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 26, 2022

The only remaining issue, is that I'd like a NEWS entry that covers both this PR and Serhiy's PR. I don't think fixing this qualifies for skip news. Serhiy?

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jul 26, 2022

At first I thought that these are rather theoretical bugs that do not manifest themselves in the real world, or the effect of which is insignificant. Later I realized that it is possible that these bugs can lead to a crash if syslog.openlog(), syslog.syslog() and syslog.closelog() are used in a multi-threaded program. And I was in such a hurry to merge #95058 to unlock #95011 that I forgot to add a NEWS entry. I still does not have a reproducer to confirm that it is a realistic danger. Maybe I'll add a NEWS entry later, maybe with some tests, if they are not too burdensome.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 26, 2022

Ok, I'm fine with that. Let's land this :)

@serhiy-storchaka serhiy-storchaka merged commit b1f648e into python:main Jul 26, 2022
12 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Jul 26, 2022

Thanks @noamcohen97 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒🤖

@miss-islington
Copy link
Contributor

miss-islington commented Jul 26, 2022

Sorry @noamcohen97 and @serhiy-storchaka, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker b1f648efc56ff17e18ec2b7402d59a771b305004 3.11

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 26, 2022
…og fails (pythonGH-95264)

(cherry picked from commit b1f648e)

Co-authored-by: Noam Cohen <noam@noam.me>
@bedevere-bot
Copy link

bedevere-bot commented Jul 26, 2022

GH-95275 is a backport of this pull request to the 3.10 branch.

@miss-islington
Copy link
Contributor

miss-islington commented Jul 26, 2022

Thanks @noamcohen97 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒🤖

@bedevere-bot
Copy link

bedevere-bot commented Jul 26, 2022

GH-95277 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 26, 2022
…og fails (pythonGH-95264)

(cherry picked from commit b1f648e)

Co-authored-by: Noam Cohen <noam@noam.me>
@erlend-aasland erlend-aasland mentioned this pull request Jul 26, 2022
@noamcohen97 noamcohen97 deleted the gh-95041-openlog-fail branch Jul 26, 2022
miss-islington added a commit that referenced this pull request Jul 26, 2022
…ls (GH-95264)

(cherry picked from commit b1f648e)

Co-authored-by: Noam Cohen <noam@noam.me>
@bedevere-bot
Copy link

bedevere-bot commented Jul 26, 2022

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Non-Debug with X 3.x has failed when building commit b1f648e.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/58/builds/2801) and take a look at the build logs.
  4. Check if the failure is related to this commit (b1f648e) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/58/builds/2801

Summary of the results of the build (if available):

== Tests result: ENV CHANGED ==

424 tests OK.

10 slowest tests:

  • test_multiprocessing_spawn: 6 min 19 sec
  • test_tokenize: 4 min 8 sec
  • test_concurrent_futures: 4 min 6 sec
  • test_asyncio: 3 min 24 sec
  • test_unparse: 2 min 48 sec
  • test_lib2to3: 2 min 25 sec
  • test_multiprocessing_fork: 1 min 48 sec
  • test_multiprocessing_forkserver: 1 min 40 sec
  • test_gdb: 1 min 39 sec
  • test_venv: 1 min 31 sec

1 test altered the execution environment:
test_import

11 tests skipped:
test_dbm_ndbm test_devpoll test_ioctl test_kqueue test_launcher
test_msilib test_startfile test_winconsoleio test_winreg
test_winsound test_zipfile64

Total duration: 40 min 57 sec

Click to see traceback logs
TracebackTests.test_unencodable_filename) ... ok


TracebackTests.test_nonexistent_module) ... ok


TracebackTests.test_broken_submodule) ... ok


TracebackTests.test_broken_from) ... ok


TracebackTests.test_exec_failure_nested) ... ok


TracebackTests.test_import_bug) ... ok


TracebackTests.test_nonexistent_module_nested) ... ok


TracebackTests.test_exec_failure) ... ok


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/threading.py", line 1036, in _bootstrap_inner
    self.run()
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/threading.py", line 973, in run
    self._target(*self._args, **self._kwargs)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/test/test_import/__init__.py", line 471, in run
    sys.settrace(None)
RuntimeError: Cannot install a trace function while another trace function is being installed
k


TracebackTests.test_broken_parent_from) ... ok


TracebackTests.test_broken_parent) ... ok


TracebackTests.test_syntax_error) ... ok

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 26, 2022

The failure in test_concurrency seems unrelated to this PR.

miss-islington added a commit that referenced this pull request Jul 27, 2022
…ls (GH-95264)

(cherry picked from commit b1f648e)

Co-authored-by: Noam Cohen <noam@noam.me>
@serhiy-storchaka serhiy-storchaka removed their assignment Sep 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news type-bug An unexpected behavior, bug, or error type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants