Skip to content

Conversation

@plafosse
Copy link
Member

@plafosse plafosse commented Nov 4, 2025

This is a fundamental rewrite of how TypeBuilder objects are handled in the python API.

TypeBuilder.handle creates an immutable_type gets the handle and then deletes the object to which the handle belonged to.
This was fundamentally a UAF. immutable_copy is an error prone api as its very easy to misuse. We should consider a re-architecture of this API. `TypeBuilder.immutable_copy().<anything>` or `Type.mutable_copy().<anything>` should be considered sketchy and immutable_copy().handle is just a straight up UAF. Prior to this commit the following would cause a crash:

current_data_variable.type = PointerBuilder.create(FunctionType.create())
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@plafosse plafosse requested a review from Copilot November 4, 2025 19:24
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a use-after-free (UAF) vulnerability by ensuring that immutable copies of Type and TypeBuilder objects are created before passing their handles to core functions. The changes prevent type objects from being garbage collected while their handles are still in use.

  • Extracts immutable_copy() calls into local variables to maintain references throughout the lifetime of handle usage
  • Adds a type_list to FunctionBuilder._to_core_struct() to preserve type object references
  • Updates _to_core_struct() return signature to include the preserved type list
  • Applies immutable copies consistently across all type-related API calls

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
python/types.py Core changes to prevent UAF by storing immutable copies before extracting handles; removed unused handle property and _to_core_struct from TypeBuilder; updated FunctionBuilder._to_core_struct to return type_list
python/mediumlevelil.py Applied immutable_copy() before calling _to_core_struct() to prevent UAF
python/highlevelil.py Applied immutable_copy() before calling _to_core_struct() to prevent UAF
python/function.py Applied immutable_copy() before accessing handles to prevent UAF across multiple function methods
python/binaryview.py Applied immutable_copy() before accessing handles; updated isinstance checks to only check for Type after conversion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +8693 to 8697
type_obj = type_obj.immutable_copy()
if not isinstance(type_obj, _types.Type):
raise TypeError("type_obj must be a Type object")
if _name is None:
raise ValueError("name can only be None if named type is derived from string passed to type_obj")
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isinstance check at line 8694 is now redundant since immutable_copy() always returns a Type object. This check should be moved before the immutable_copy() call to validate the input parameter, or the error message should be updated to reflect that the conversion failed. The same pattern occurs at lines 8728-8730.

Suggested change
type_obj = type_obj.immutable_copy()
if not isinstance(type_obj, _types.Type):
raise TypeError("type_obj must be a Type object")
if _name is None:
raise ValueError("name can only be None if named type is derived from string passed to type_obj")
elif not isinstance(type_obj, _types.Type):
raise TypeError("type_obj must be a Type object or a string")
elif not isinstance(type_obj, _types.Type):
raise TypeError("type_obj must be a Type object or a string")
type_obj = type_obj.immutable_copy()

Copilot uses AI. Check for mistakes.
Comment on lines +8728 to 8730
type_obj = type_obj.immutable_copy()
if not isinstance(type_obj, _types.Type):
raise TypeError("type_obj must be a Type object")
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isinstance check at line 8729 is now redundant since immutable_copy() always returns a Type object. This check should be moved before the immutable_copy() call to validate the input parameter, or the error message should be updated to reflect that the conversion failed. This is the same issue as in export_type_to_library method.

Suggested change
type_obj = type_obj.immutable_copy()
if not isinstance(type_obj, _types.Type):
raise TypeError("type_obj must be a Type object")
elif not isinstance(type_obj, _types.Type):
raise TypeError("type_obj must be a Type object or a type string")
type_obj = type_obj.immutable_copy()

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants