Repository navigation
Add a test case that prevents magic number changes in minor releases #73700
Description
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']
Activity
ncoghlan commentedon Feb 9, 2017
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:
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 commentedon Feb 9, 2017
For the record, the magic number was changed in bpo-27286
brettcannon commentedon 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 commentedon Feb 10, 2017
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 commentedon 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 commentedon Feb 10, 2017
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 commentedon Feb 10, 2017
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 commentedon Feb 10, 2017
On Feb 10, 2017, at 05:46 PM, Nick Coghlan wrote:
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 commentedon Feb 10, 2017
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 commentedon Feb 10, 2017
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 commentedon 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 commentedon 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 commentedon Feb 11, 2017
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 commentedon 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.
28 remaining items
ncoghlan commentedon Jun 13, 2017
New changeset d071a20 by Nick Coghlan in branch '3.5':
[3.5] bpo-29514: Check magic number for bugfix releases (bpo-2158)
d071a20
ncoghlan commentedon Jun 13, 2017
New changeset 92e9e35 by Nick Coghlan in branch '3.6':
[3.6] bpo-29514: Check magic number for bugfix releases (bpo-2157)
92e9e35
ncoghlan commentedon Jun 13, 2017
New changeset a51f12f by Nick Coghlan in branch '2.7':
[2.7] bpo-29514: Check magic number for bugfix releases
a51f12f
ncoghlan commentedon Jun 13, 2017
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 commentedon 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 commentedon Jun 15, 2017
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 commentedon Jun 27, 2018
New changeset e7d3ccc by Ned Deily in branch 'master':
bpo-29514: Make magic number test work for candidates
e7d3ccc
ned-deily commentedon Jun 27, 2018
New changeset 8511b44 by Ned Deily in branch '3.7':
bpo-29514: Make magic number test work for candidates
8511b44
ned-deily commentedon Jun 27, 2018
New changeset 57389db by Ned Deily in branch '3.6':
bpo-29514: Make magic number test work for candidates (GH-7945)
57389db
ned-deily commentedon Jun 27, 2018
New changeset f79386f by Ned Deily in branch '2.7':
bpo-29514: Make magic number test work for candidates (GH-7946)
f79386f
ned-deily commentedon Jun 27, 2018
New changeset 8283517 by Ned Deily in branch '3.7':
bpo-29514: Make magic number test work for candidates
8283517