Skip to content

bpo-43132: Fix incorrect handling of PyObject_RichCompareBool() in _zoneinfo #24450

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

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Feb 4, 2021

PyObject_RichCompareBool() returns -1 on error, but this case is
not handled by the find_in_strong_cache() function. Any exception
raised by PyObject_RichCompareBool() should be propagated.

https://bugs.python.org/issue43132

…oneinfo

PyObject_RichCompareBool() returns -1 on error, but this case is
not handled by the find_in_strong_cache() function.  Any exception
raised by PyObject_RichCompareBool() should be propagated.
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@serhiy-storchaka serhiy-storchaka added needs backport to 3.9 only security fixes type-bug An unexpected behavior, bug, or error skip news labels Feb 5, 2021
@serhiy-storchaka serhiy-storchaka merged commit effaec0 into python:master Feb 5, 2021
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-24457 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Feb 5, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 5, 2021
…oneinfo (pythonGH-24450)

PyObject_RichCompareBool() returns -1 on error, but this case is
not handled by the find_in_strong_cache() function.  Any exception
raised by PyObject_RichCompareBool() should be propagated.
(cherry picked from commit effaec0)

Co-authored-by: Zackery Spytz <[email protected]>
@pganssle
Copy link
Member

pganssle commented Feb 5, 2021

Um... I think this could use tests, no?

serhiy-storchaka pushed a commit that referenced this pull request Feb 5, 2021
…oneinfo (GH-24450) (GH-24457)

PyObject_RichCompareBool() returns -1 on error, but this case is
not handled by the find_in_strong_cache() function.  Any exception
raised by PyObject_RichCompareBool() should be propagated.
(cherry picked from commit effaec0)

Co-authored-by: Zackery Spytz <[email protected]>
@bedevere-bot
Copy link

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

Hi! The buildbot PPC64LE RHEL8 3.9 has failed when building commit c8b4375.

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/250/builds/251) and take a look at the build logs.
  4. Check if the failure is related to this commit (c8b4375) 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/250/builds/251

Failed tests:

  • test_asyncio

Failed subtests:

  • test_shell - test.test_asyncio.test_subprocess.SubprocessMultiLoopWatcherTests

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

== Tests result: FAILURE then FAILURE ==

411 tests OK.

10 slowest tests:

  • test_concurrent_futures: 4 min 6 sec
  • test_tokenize: 3 min 26 sec
  • test_gdb: 3 min 21 sec
  • test_peg_generator: 3 min
  • test_unparse: 2 min 48 sec
  • test_multiprocessing_spawn: 2 min 25 sec
  • test_lib2to3: 2 min 16 sec
  • test_capi: 1 min 46 sec
  • test_unicodedata: 1 min 37 sec
  • test_multiprocessing_forkserver: 1 min 34 sec

1 test failed:
test_asyncio

13 tests skipped:
test_devpoll test_ioctl test_kqueue test_msilib test_ossaudiodev
test_startfile test_tix test_tk test_ttk_guionly test_winconsoleio
test_winreg test_winsound test_zipfile64

1 re-run test:
test_asyncio

Total duration: 21 min 51 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.9.cstratak-RHEL8-ppc64le/build/Lib/asyncio/unix_events.py", line 1306, in _do_waitpid
    loop, callback, args = self._callbacks.pop(pid)
KeyError: 3270825


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.9.cstratak-RHEL8-ppc64le/build/Lib/test/test_asyncio/test_subprocess.py", line 157, in test_shell
    self.assertEqual(exitcode, 7)
AssertionError: 255 != 7

@pganssle
Copy link
Member

pganssle commented Feb 5, 2021

I think you can trigger the find_in_strong_cache bug easily enough by creating a class where __eq__ raises an exception and trying to construct a zoneinfo.ZoneInfo from it:

class ComparisonError(Exception):
    pass

class Incomparable:
    def __eq__(self, other):
        raise ComparisonError

key = Incomparable()
with self.assertRaises(ComparisonError):
    zoneinfo.ZoneInfo(key)

The other path I think is trickier to hit, and the consequence is a refleak, not a SystemError, but I think you can hit it like this:

class Stringy(str):
    def __new__(cls, value):
        rv = super().__new__(cls, value)
        rv.allow_comparisons = True
    def __eq__(self, other):
        if not self.allow_comparisons:
            raise ComparisonError
        return super().__eq__(other)
    def __hash__(self):
        return hash(self[:])

key = Stringy("America/New_York")
zoneinfo.ZoneInfo(key)
key.allow_comparisons = False
try:
    # Note: This is try/except rather than assertRaises because there is no guarantee
    # that the key is even still in the cache, or that the key for the cache is the original
    # `key` object.
    zoneinfo.ZoneInfo.clear_cache(only_keys="America/New_York")
except ComparisonError:
    pass

A possible alternative solution in the eject_from_strong_cache case might be to modify zoneinfo_new so that when it is given a subclass of str, the result is first cast to an str, guaranteeing that all strong cache keys are str objects and possibly making it impossible to hit that error case in eject_from_strong_cache (though admittedly it might be good to do the check anyway...).

I guess we'll need a separate PR to add the tests, though. ☹

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…oneinfo (pythonGH-24450)

PyObject_RichCompareBool() returns -1 on error, but this case is
not handled by the find_in_strong_cache() function.  Any exception
raised by PyObject_RichCompareBool() should be propagated.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants