| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Follow-up to theupdateframework#2973, which fixed Role.__hash__ and DelegatedRole.__hash__. The same bug remains in the other implementations: Signed, Root, MetaFile, Snapshot, Delegations, TargetFile, Targets and Metadata all pass a raw dict to hash(), so hash() raises "TypeError: unhashable type: 'dict'". Timestamp.__hash__ is itself correct but inherits the failure from Signed and MetaFile. All of these classes define __eq__, so __hash__ is required for them to be usable in a set or as a dict key. test_metadata_eq_.py covers __eq__ for exactly these classes but never calls hash(), which is why this went unnoticed. Hash a subset of immutable fields, as theupdateframework#2973 did. unrecognized_fields is excluded throughout since it holds arbitrary nested JSON. Snapshot.meta and Targets.targets contribute len() rather than their keys, to keep hashing O(1) for roles with many entries. Signed-off-by: Sachith Reddy <sachith.24bcs10403@sst.scaler.com>
Signed-off-by: Sachith Reddy <sachith.24bcs10403@sst.scaler.com>
|
Yeah, both are fair. hashes is just dict[str, str] so there was no real reason to drop it, I was being over-careful. And with len(), two snapshots with the same number of entries ended up with the same hash. Used tuple(sorted(...)) in all four since a dict isn't hashable on its own, and the sort keeps it stable if the key order varies. |
Sorry, something went wrong.
There was a problem hiding this comment.
I spent a bit more time and I think it's still not complete, details in code.
Unfortunately several cases will keep failing even with (what I think is) correct code as the securesystemslib hashing seems to still not be complete. I'm fine with either
Sorry, something went wrong.
Co-authored-by: Jussi Kukkonen <jku@goto.fi> Signed-off-by: Sanigaram Sachith Reddy <sachith.24bcs10403@sst.scaler.com>
Co-authored-by: Jussi Kukkonen <jku@goto.fi> Signed-off-by: Sanigaram Sachith Reddy <sachith.24bcs10403@sst.scaler.com>
Co-authored-by: Jussi Kukkonen <jku@goto.fi> Signed-off-by: Sanigaram Sachith Reddy <sachith.24bcs10403@sst.scaler.com>
Co-authored-by: Jussi Kukkonen <jku@goto.fi> Signed-off-by: Sanigaram Sachith Reddy <sachith.24bcs10403@sst.scaler.com>
Co-authored-by: Jussi Kukkonen <jku@goto.fi> Signed-off-by: Sanigaram Sachith Reddy <sachith.24bcs10403@sst.scaler.com>
Signed-off-by: Sachith Reddy <sachith.24bcs10403@sst.scaler.com>
|
Thanks for the detailed review. I made the same assumption as before with the dictionaries , focused on making them hashable and initially overlooked that the values also need to be included. I've applied all the suggestions. I added a runtime check in the tests so Root/Delegations/Metadata are skipped while Key and Signature aren't hashable, and will start running once the sslib update is available. I also tested it locally against sslib main, and those three tests pass there. |
Sorry, something went wrong.
👍 thanks for testing |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Description
Follow-up to #2973, which fixed __hash__ for Role and DelegatedRole.
Same problem was still there in the other metadata classes: Signed, Root, MetaFile, Snapshot, Delegations, TargetFile, Targets, and Metadata. Their __hash__ implementations passed raw dictionaries into hash(), which throws TypeError: unhashable type: 'dict'.
Timestamp.__hash__ looked fine on its own, but still breaks because it inherits from Signed and MetaFile.
All these classes define __eq__, so they need a working __hash__ too if they're going to be used in sets or as dict keys. test_metadata_eq_.py covers equality for these classes but never tested hashing, which is probably why this slipped through.
Fix follows the same approach as #2973 — hash a subset of immutable fields instead of the raw dicts.
unrecognized_fields is left out since it can hold arbitrary nested JSON. Everything else I just converted to tuples.
Testing