Fix locale data cross-contamination via shared, mutated alias caches - #1294
Open
agu2347 wants to merge 1 commit into
Open
Fix locale data cross-contamination via shared, mutated alias caches#1294agu2347 wants to merge 1 commit into
agu2347 wants to merge 1 commit into
Conversation
Using one locale (e.g. 'he') could silently corrupt subsequently-used, completely unrelated locales (e.g. 'no', 'fr') for the remainder of the process, causing them to return the first locale's resolved text instead of their own -- persisting until the process restarts. Two existing, individually-reasonable optimizations combine to cause this: 1. localedata.load() gives each locale its own top-level dict via a shallow `load(parent).copy()` of its parent's cached data. merge() then only copies (and recurses into) a nested dict when the locale's own data file actually has an entry at that key. A nested structure the locale's data file doesn't touch at all -- e.g. a "stand-alone" month-name alias, when a locale relies entirely on the CLDR-inherited default rather than defining its own -- therefore remains the exact same dict object shared with whatever it was last inherited from, and consequently with every other locale that also inherits it unchanged. 2. LocaleDataDict.__getitem__ has a "cache the resolved alias value back into the dict" optimization, so repeated lookups of the same key don't need to re-resolve the alias every time. It writes this resolved value directly into self._data[key]. Put together: resolving a shared, not-locally-overridden alias for one locale permanently overwrites the shared dict with that locale's own resolved value. The next locale that looks up the same key -- sharing the exact same (now-overwritten) dict object -- sees the previous locale's resolved value instead of correctly re-resolving for itself. Give LocaleDataDict a copy-on-write: the first time __getitem__ needs to cache a resolved value into self._data, copy self._data first (the same way merge() already copies a nested dict before recursing into and mutating it), and remember that this instance now owns an independent copy so subsequent writes don't re-copy needlessly. This ensures resolving an alias for one locale can never affect any other LocaleDataDict that happens to still be sharing the same underlying dict object. Verified against the exact reproduction from the issue (installed a release build with prebuilt CLDR data, since building the data files from source requires network access to unicode.org this sandbox doesn't have, and overlaid the fixed localedata.py onto it): confirmed 'no' and 'fr' month names were previously corrupted to Hebrew text after formatting a date with 'he', and are correctly independent with the fix. Ran a broader sweep across 20 locales in both forward and reverse order, and across two different fields (month names and weekday names): 9 of 12 checked locales were corrupted without the fix (a wider-reaching bug than the original report's specific locales), and all are correct with the fix, in every ordering tested. Added a focused unit test reproducing the precise structural condition that causes this (a shared, unmutated "stand-alone" alias structure alongside independent, locale-specific "format" data -- the same shape real CLDR data has for many locales), without requiring built locale data files. Confirmed the test fails with the original code (one locale's resolved value leaks into the other's lookup) and passes with the fix. Ran the full existing test_localedata.py suite (15 passed: 14 baseline + 1 new) and the broader test_dates.py and test_core.py suites (3428 passed total, 2 pre-existing zoneinfo-data failures unrelated to this change, confirmed identical with and without the fix). Fixes python-babel#1234
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1234.
Using one locale (e.g.
'he') could silently corrupt subsequently-used, completely unrelated locales (e.g.'no','fr') for the remainder of the process, causing them to return the first locale's resolved text instead of their own -- persisting until the process restarts. Exactly as described in the issue.Root cause: two existing, individually-reasonable optimizations combine to cause this:
localedata.load()gives each locale its own top-level dict via a shallowload(parent).copy()of its parent's cached data.merge()then only copies (and recurses into) a nested dict when the locale's own data file actually has an entry at that key. A nested structure the locale's data file doesn't touch at all -- e.g. a "stand-alone" month-name alias, when a locale relies entirely on the CLDR-inherited default rather than defining its own -- therefore remains the exact same dict object shared with whatever it was last inherited from, and consequently with every other locale that also inherits it unchanged.LocaleDataDict.__getitem__has a "cache the resolved alias value back into the dict" optimization, so repeated lookups of the same key don't need to re-resolve the alias every time. It writes this resolved value directly intoself._data[key].Put together: resolving a shared, not-locally-overridden alias for one locale permanently overwrites the shared dict with that locale's own resolved value. The next locale that looks up the same key -- sharing the exact same (now-overwritten) dict object -- sees the previous locale's resolved value instead of correctly re-resolving for itself.
Fix: give
LocaleDataDicta copy-on-write: the first time__getitem__needs to cache a resolved value intoself._data, copyself._datafirst (the same waymerge()already copies a nested dict before recursing into and mutating it), and remember that this instance now owns an independent copy so subsequent writes don't re-copy needlessly. This ensures resolving an alias for one locale can never affect any otherLocaleDataDictthat happens to still be sharing the same underlying dict object.Testing: verified against the exact reproduction from the issue (installed a release build with prebuilt CLDR data, since building the data files from source requires network access to unicode.org this sandbox doesn't have, and overlaid the fixed
localedata.pyonto it): confirmed'no'and'fr'month names were previously corrupted to Hebrew text after formatting a date with'he', and are correctly independent with the fix. Ran a broader sweep across 20 locales in both forward and reverse order, and across two different fields (month names and weekday names): 9 of 12 checked locales were corrupted without the fix (a wider-reaching bug than the original report's specific locales), and all are correct with the fix, in every ordering tested.Added a focused unit test reproducing the precise structural condition that causes this (a shared, unmutated "stand-alone" alias structure alongside independent, locale-specific "format" data -- the same shape real CLDR data has for many locales), without requiring built locale data files. I confirmed the test fails with the original code (one locale's resolved value leaks into the other's lookup) and passes with the fix. Ran the full existing
test_localedata.pysuite (15 passed: 14 baseline + 1 new) and the broadertest_dates.pyandtest_core.pysuites (3428 passed total, 2 pre-existing zoneinfo-data failures unrelated to this change, confirmed identical with and without the fix).