gemini-code-assist[bot] commented on code in PR #592:
URL: https://github.com/apache/tvm-ffi/pull/592#discussion_r3264585086
##########
src/ffi/error.cc:
##########
@@ -31,12 +31,25 @@ namespace ffi {
class SafeCallContext {
public:
void SetRaised(TVMFFIObjectHandle error) {
+ // Explicitly zero cause_chain and extra_context before storing in TLS.
+ // On macOS arm64 the allocator may reuse memory that contains stale
+ // non-null bytes, which would cause ~ErrorObj() to call DecRefObjectHandle
+ // on a garbage pointer → SIGBUS (alignment fault).
+ auto* cell = TVMFFIErrorGetCellPtr(error);
+ cell->cause_chain = nullptr;
+ cell->extra_context = nullptr;
Review Comment:
Unconditionally zeroing `cause_chain` and `extra_context` in `SetRaised`
will overwrite valid data if the `error` object was already initialized with a
cause chain (e.g., via `TVMFFIErrorCreateWithCauseAndExtraContext`). This
results in data loss and potential memory leaks of the objects previously held
in those fields.
If these fields contain garbage due to a layout mismatch or uninitialized
memory in the allocator, the root cause should ideally be addressed in the
`ErrorObj` constructor in `include/tvm/ffi/error.h`. This would ensure all
`ErrorObj` instances are correctly initialized upon creation, regardless of how
they are subsequently used, and would avoid the need for manual zeroing in
every call site.
##########
src/ffi/error.cc:
##########
@@ -31,12 +31,25 @@ namespace ffi {
class SafeCallContext {
public:
void SetRaised(TVMFFIObjectHandle error) {
+ // Explicitly zero cause_chain and extra_context before storing in TLS.
+ // On macOS arm64 the allocator may reuse memory that contains stale
+ // non-null bytes, which would cause ~ErrorObj() to call DecRefObjectHandle
+ // on a garbage pointer → SIGBUS (alignment fault).
+ auto* cell = TVMFFIErrorGetCellPtr(error);
+ cell->cause_chain = nullptr;
+ cell->extra_context = nullptr;
last_error_ =
details::ObjectUnsafe::ObjectPtrFromUnowned<ErrorObj>(static_cast<TVMFFIObject*>(error));
}
void SetRaisedByCstr(const char* kind, const char* message, const
TVMFFIByteArray* backtrace) {
Error error(kind, message, backtrace);
+ // Zero the newly created Error's cause_chain / extra_context as defensive
+ // measure (see comment in SetRaised above).
+ auto* cell = TVMFFIErrorGetCellPtr(
+ details::ObjectUnsafe::TVMFFIObjectPtrFromObjectRef(error));
+ cell->cause_chain = nullptr;
+ cell->extra_context = nullptr;
last_error_ =
details::ObjectUnsafe::ObjectPtrFromObjectRef<ErrorObj>(std::move(error));
Review Comment:
The fix applied here should also be extended to `SetRaisedByCstrParts`
(lines 56-73) to ensure consistency across all error-raising methods in
`SafeCallContext`. Currently, `SetRaisedByCstrParts` remains vulnerable to the
same SIGBUS issue described in the PR as it also creates a new `Error` and
stores it in `last_error_` without zeroing these fields.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
