Fixes dict_hash discrepancy #3195
Conversation
| "feature_proc_columns": {feature[PROC_COLUMN] for feature in features}, | ||
| # creating a sorted list out of the dict because hash_dict requires all values | ||
| # of the dict to be ordered object to ensure the creation fo the same hash | ||
| "feature_proc_columns": sorted({feature[PROC_COLUMN] for feature in features}), |
There was a problem hiding this comment.
I'm wondering if there's a way we can test this. Something that would force the behavior of changing the insertion order of the set.
This looks promising:
By default, the hash() values of str, bytes and datetime objects are “salted” with an unpredictable random value. Although they remain constant within an individual Python process, they are not predictable between repeated invocations of Python.
This is intended to provide protection against a denial-of-service caused by carefully-chosen inputs that exploit the worst case performance of a dict insertion, O(n^2) complexity. See http://www.ocert.org/advisories/ocert-2011-003.html for details.
Changing hash values affects the iteration order of dicts, sets and other mappings. Python has never made guarantees about this ordering (and it typically varies between 32-bit and 64-bit builds).
See also PYTHONHASHSEED.
There was a problem hiding this comment.
Let me see if I can put a test that exploits this together and add it to this PR.
There was a problem hiding this comment.
Okay, added a test and verified it repros the issue without this fix, and with this fix succeeds.
There was a problem hiding this comment.
Nice, thanks for looking into Python docs @tgaddair. It makes sense that the ordering of sets would cause some problems from the docs, so this is a good change!
Also pretty interesting that the salts are scoped to individual processes rather than across processes, but it makes sense if you think about it
There was a problem hiding this comment.
If the salts were the same between processes, then it wouldn't serve its purpose of making the hash function unpredictable (to an attacker). But if they were different within a process, then hash lookups wouldn't work ;). So it makes sense, just never considered that they designed their hash function with that exploit in mind.
Co-authored-by: Travis Addair <tgaddair@gmail.com>
Co-authored-by: Travis Addair <tgaddair@gmail.com>
No description provided.