GeezerVison is showing github.com — Add a test case that prevents magic number changes in minor releases · Issue #73700 · python/cpython
Add a test case that prevents magic number changes in minor releases #73700Description
| BPO |
29514 |
| Nosy |
@warsaw, @brettcannon, @doko42, @mdickinson, @ncoghlan, @ned-deily, @encukou, @ambv, @ericsnowcurrently, @serhiy-storchaka, @appeltel, @irushchyshyn |
| PRs |
bpo-29514: Check magic number for bugfix release #54[3.6] bpo-29514: Check magic number for bugfix releases #2157[3.5] bpo-29514: Check magic number for bugfix releases #2158[2.7] bpo-29514: Check magic number for bugfix releases #2160bpo-29514: Make magic number test work for candidates #7945bpo-29514: Make magic number test work for candidates #7946 |
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 = 'https://github.com/ncoghlan'
closed_at = <Date 2017-06-13.11:02:59.254>
created_at = <Date 2017-02-09.15:30:10.369>
labels = ['type-feature']
title = 'Add a test case that prevents magic number changes in minor releases'
updated_at = <Date 2018-06-27.22:34:50.190>
user = 'https://github.com/ncoghlan' bugs.python.org fields:
activity = <Date 2018-06-27.22:34:50.190>
actor = 'ned.deily'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2017-06-13.11:02:59.254>
closer = 'ncoghlan'
components = []
creation = <Date 2017-02-09.15:30:10.369>
creator = 'ncoghlan'
dependencies = []
files = []
hgrepos = []
issue_num = 29514
keywords = []
message_count = 52.0
messages = ['287428', '287429', '287551', '287553', '287554', '287557', '287558', '287560', '287561', '287562', '287563', '287585', '287599', '287611', '287619', '287622', '287624', '287642', '287646', '287647', '287650', '287652', '287654', '287656', '287659', '287665', '287670', '287671', '287672', '287676', '287678', '287679', '287694', '287697', '287713', '288117', '289207', '289455', '291808', '295039', '295830', '295887', '295888', '295893', '295895', '296016', '296058', '320533', '320537', '320538', '320539', '320633']
nosy_count = 13.0
nosy_names = ['barry', 'brett.cannon', 'doko', 'mark.dickinson', 'ncoghlan', 'ned.deily', 'petr.viktorin', 'lukasz.langa', 'eric.snow', 'serhiy.storchaka', 'Carson Lam', 'Eric Appelt', 'ishcherb']
pr_nums = ['54', '2157', '2158', '2160', '7945', '7946']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue29514'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6'] Activityncoghlan commented on Feb 9, 2017 ContributorAuthor More actions The magic number change in 3.5.3 broke the world when Fedora attempted to rebase from 3.5.2, but upstream CPython developers don't currently get an automated notification that it isn't permitted to change the magic number in a maintenance release - instead, the current approach relies on people "just knowing" not to do that, and we just saw the limitations of relying on the code review process to catch this particular problem.
I think we can keep this from happening again by having an importlib test case that fails if sys.version_info.releaselevel is "candidate" or "final" and either:
- no "expected magic number" is defined; or
- the actual magic number in importlib.util.MAGIC_NUMBER doesn't match the expected one
The comments on the new test case would then explain the policy and why it isn't OK to break pyc file compatibility in a maintenance release.
We'd also propose an update to the release PEP warning release managers to expect this error as part of prepping the first release candidate, and that they should lock in the magic number for the release at that time. encukou commented on Feb 9, 2017 For the record, the magic number was changed in bpo-27286 brettcannon commented on Feb 10, 2017 Why shouldn't the magic number be bumped when a bug in the bytecode is fixed in a bugfix release? How did this break Fedora? Are you not shipping source code but only .pyc files?
I view the whole point of having the magic number is to cover things like this, otherwise we might as well just embed the CPython version number in the file name and be done with it. ncoghlan commented on Feb 10, 2017 ContributorAuthor More actions There are a couple of pieces to it:
-
first, there's the problem of breaking anyone that actually is doing pyc-only distribution, even though nothing in Fedora itself does that. While that aspect doesn't break Fedora per se, it does break Fedora's compatibility expectations for end user components, which maintenance updates shouldn't do
-
however, more importantly in this case, ordinary users don't have write access to the system site-packages directory, so the pyc files have to be pre-compiled when generating the system RPMs.
While CPython falls back to the slow path of recompiling from source when those precompiled files are invalidated so nothing obviously breaks, it still generates a lot of SELinux warnings related to unauthorised write attempts to system-owned directories: https://bodhi.fedoraproject.org/updates/python3-3.5.3-1.fc25%20python3-docs-3.5.3-1.fc25#comment-559458
(Debian uses a similar pre-compilation trick for the same reasons, so I expect you'd also see comparable warnings under AppArmor if packages weren't rebuilt after a magic number change)
This all means that CPython changing the magic number in a maintenance release ends up being akin to breaking the C ABI in a maintenance release - while it isn't quite as dire (since there's still the slow path fallback to recompiling from source), it does still invalidate all of the pyc files in the system RPMs until they're rebuilt against the new version. "Requires a mass rebuild of all the Python packages in the distro to get things working properly again" is again a major concern for what's nominally a low impact maintenance update.
I realise those consequences aren't particularly obvious for non-distro developers, which is why I'd like to see the previous policy of "don't change the magic number in maintenance releases" captured as a test case. brettcannon commented on Feb 10, 2017 OK, but how would you propose to fix broken bytecodes in a bugfix be fixed if you can't bump the magic number? And why aren't you regenerating the bytecode when updating Python or shipping the bytecode with the install (and I bet the answer is "RPM doesn't support it" and "it goes against packaging guidelines to include auto-generated files", but I thought I would ask :) . ncoghlan commented on Feb 10, 2017 ContributorAuthor More actions We *do* ship the bytecode in the built RPMs - the issue is that Python maintenance releases shouldn't be triggering rebuilds of thousands of system components (cf http://fedora.portingdb.xyz/ ) and potentially breaking otherwise working end user applications.
As far as "What about bytecode generation bugs?" goes, then my answer would be to find a way to fix the bug for freshly generated bytecode, without forcing stale bytecode to be regenerated. That way folks that actually hit the original bug have a way of solving it (clear their bytecode cache), while we also don't forcibly break the world.
That is, the problem with the resolution of bpo-27836 wasn't fixing the bytecode generation when using double-star unpacking twice in a single function call - it was forcing the regeneration of *all* bytecode just because *some* bytecode would be broken if it used a particular new language feature. It's the equivalent of deliberately breaking the ABI for all extension modules just because we found a header bug affecting one particular function signature. ncoghlan commented on Feb 10, 2017 ContributorAuthor More actions Note that Fedora doesn't even rebuild all the extension modules when bumping CPython to a new maintenance release, let alone rebuilding and re-releasing all the pure Python ones. (RPM supports doing that just fine, but it would mean shipping thousands of updated binary artifacts instead of just one - the new CPython maintenance release) warsaw commented on Feb 10, 2017 On Feb 10, 2017, at 05:46 PM, Nick Coghlan wrote:
Note that Fedora doesn't even rebuild all the extension modules when bumping
CPython to a new maintenance release, let alone rebuilding and re-releasing
all the pure Python ones. (RPM supports doing that just fine, but it would
mean shipping thousands of updated binary artifacts instead of just one - the
new CPython maintenance release)
Debian/Ubuntu doesn't rebuild extension modules either. We don't ship .pyc
files in binary packages, but instead build them at install time on the user's
machine. encukou commented on Feb 10, 2017
How would you propose to fix broken bytecodes in a bugfix be fixed if you can't bump the magic number?
That would depend on the situation. I can imagine that if the bug is severe enough, the number could be bumped, after careful discussion, and with the change being advertised rather loudly.
The test case that's proposed can be changed if it's indeed the best thing to do, but it should at the very least be mentioned in release notes.
FWIW, I'm not convinced the bug here was severe enough. Practically no one is using the 3.5+ syntax in libraries: we checked to find that *zero* packages in Fedora are using the affected opcode. But that's moot now – wider discussion on that should have happened before the release. ncoghlan commented on Feb 10, 2017 ContributorAuthor More actions Ah, interesting. The install-time generation case is even trickier, since you presumably can't force in-place regeneration of existing pyc files on systems that previously had 3.5.2 and upgraded to 3.5.3.
When I last spoke to Peter about this yesterday, he was exploring the idea of a downstream-only patch to 3.5.3 that gave it the ability to load pyc files that used the 3.5.2 magic number (so it would *emit* files with the new magic number, but still allow use of pyc files with the old one). Assuming he can get that to work, then I guess it may be of interest to Debian & Ubuntu as well. encukou commented on Feb 10, 2017 According to bpo-27286, Ubuntu 16.04LTS has the fix and uses the new magic number. (And this is one reason undoing the change in CPython is out of the question.)
The patch Nick mentioned is brewing at encukou@magic-number-workaround . ericsnowcurrently commented on Feb 11, 2017 This is strictly a problem for the system Python, right? In that case, can't the dist package clear __pycache__ under the system site-packages directory (and any other user-read-only dirs) during install of the updated Python?
Is the concern that upgrading Python may force all .pyc files to be re-written (perhaps unnecessarily)? If so, I'm not clear on why it's okay between major versions but not minor ones. Frequency perhaps? Is the cost of re-compiling all .pyc files (which mostly won't happen all at the same time) significant enough to warrant changing the status quo, particularly since the magic number rarely changes in a minor release?
As one (poor) alternative, we could require re-compiling a .pyc file only if it contains the affected bytecode(s). Granted, that would probably require associating every bytecode with a magic number. The required comparison would likely be more expensive than just re-compiling. :/
Another alternative would be to leverage sys.dont_write_bytecode somehow. ncoghlan commented on Feb 11, 2017 ContributorAuthor More actions Providing some additional background:
-
3.5.3 was the *first time ever* in more than two decades that CPython changed the magic number in a maintenance release. It *wasn't* necessary to do so - it could have been left alone (as 3.5.0, 3.5.1, and 3.5.2 can read the revised bytecode just fine - no changes were made to ceval, only to the code generator). If the magic number had been left alone, recompilation would have been limited solely to folks with affected bytecode files, which is approximately zero people in practice (as anyone impacted by the problem would have been getting weird runtime behaviour and presumably found a way to work around it).
-
because the pyc caches for system packages on Linux are in system-owned directories, they're generated either at build time (Fedora et al) or install time (Debian et al) by the package that owns the corresponding source files and then installed with elevated privileges (e.g. "sudo dnf install python-requests"). These files *do not* belong to the CPython package, they belong to the individual Python modules that depend on CPython, so messing with them when installing a new version of CPython would be a significant breach of distro policies
-
as a result, removing CPython 3.5.3's ability to read pyc files generated by 3.5.0, 3.5.1 or 3.5.2 means that either the distros have to regenerate all of their Python dependent packages (to force recompilation of the affected bytecode files), or else patch the system Python so it can still read the old pyc files (which is the approach Peter is currently pursuing, since requiring a mass rebuild for a maintenance update is *also* against distro policy)
Thus this request to take the previously unwritten de facto policy (which worked fine and clearly isn't a major limitation on CPython maintainability, since the one time it has been done in 20+ years didn't actually need to do it) and make it an explicit official policy that's enforced by the test suite.
The reason that none of these concerns apply to Python feature releases is because those are only rolled out as part of distro version upgrades (e.g. the switch from 3.5 to 3.6 as the system Python is one of the upgrades going from F25 to F26), where both distro maintainers and distro users already expect to have to rebuild and reinstall almost everything. serhiy-storchaka commented on Feb 11, 2017 My apologies for breaking the world. But I considered this bug as security issue. Unlikely, but not impossible. The BUILD_MAP_UNPACK_WITH_CALL opcode is rarely used, but if it is used, it can cause reading arbitrary memory and either crashing or even exposing the content of this memory in error message that can be leaked to remote user. This hole would be better to close early. I expected this change would be included in 3.5.2.
The more robust solution would be to add the support of two magic numbers and check on loading whether the BUILD_MAP_UNPACK_WITH_CALL opcode is occurred in a precompiled file with old magic number. But this solution looked too excessive.
It would be nice to design a mechanism for possible future bytecode fixes. ncoghlan commented on Jun 13, 2017 ContributorAuthor More actions ncoghlan commented on Jun 13, 2017 ContributorAuthor More actions ncoghlan commented on Jun 13, 2017 ContributorAuthor More actions New changeset a51f12f by Nick Coghlan in branch '2.7':
[2.7] bpo-29514: Check magic number for bugfix releases
a51f12f ncoghlan commented on Jun 13, 2017 ContributorAuthor More actions This has now been backported to all active maintenance branches.
For 2.7, I backported the test as a standalone one in Lib/test/test_import_magic.py, since there's only a partial importlib implementation in 2.7, and hence no test_importlib directory.
Thanks all! serhiy-storchaka commented on Jun 14, 2017 Is it intentionally that old magic number is used in 3.7? The test will become failing just after reaching the RC stage. ncoghlan commented on Jun 15, 2017 ContributorAuthor More actions Serhiy: I believe that magic number was current when the test was written, and it hasn't been getting updated since the test is currently being skipped.
Given how we designed the skip logic, I think that counts as "working as intended" - we don't really care about magic number stability until the release candidate stage, so that's when the test needs to be updated to match the implementation. ned-deily commented on Jun 27, 2018 New changeset e7d3ccc by Ned Deily in branch 'master':
bpo-29514: Make magic number test work for candidates
e7d3ccc ned-deily commented on Jun 27, 2018 ned-deily commented on Jun 27, 2018 ned-deily commented on Jun 27, 2018 ned-deily commented on Jun 27, 2018
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|