refactor(storage): add checksum validation robustness and leak prevention#17666
refactor(storage): add checksum validation robustness and leak prevention#17666chandra-siri wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an enable_checksum option to the asynchronous appendable object writer and writes resumption strategy, allowing users to disable CRC32C checksum calculation. It also adds validation for full_object_checksum in finalize and wraps the finalization response handling in a try...finally block to guarantee stream closure and state reset on failure. Unit tests have been added to cover these changes. Feedback is provided regarding the type check for full_object_checksum in finalize, noting that isinstance(..., int) will evaluate to True for boolean values because bool is a subclass of int in Python, and suggesting an explicit check to reject booleans.
| if not isinstance(full_object_checksum, int): | ||
| raise TypeError("full_object_checksum must be an integer.") |
There was a problem hiding this comment.
In Python, bool is a subclass of int, so isinstance(True, int) evaluates to True. If a boolean value is passed as full_object_checksum, it will bypass this type check and be treated as a valid checksum (e.g., 1 or 0).
To prevent this, explicitly reject boolean values.
| if not isinstance(full_object_checksum, int): | |
| raise TypeError("full_object_checksum must be an integer.") | |
| if not isinstance(full_object_checksum, int) or isinstance(full_object_checksum, bool): | |
| raise TypeError("full_object_checksum must be an integer.") |
Validate type/range of full_object_checksum and wrap stream finalization in try...finally to prevent stream leaks on error.