From 8709d3484c4dd163f3d9dc04b98acb5b60bd27f6 Mon Sep 17 00:00:00 2001 From: Ge Wang Date: Thu, 28 Nov 2024 02:13:14 -0800 Subject: [PATCH] fix object instantiation refcount integrity --- src/core/chuck_emit.cpp | 6 +- src/core/chuck_instr.cpp | 100 ++++++++++++++++-- src/core/chuck_instr.h | 28 ++++- src/core/chuck_oo.cpp | 13 +++ src/core/chuck_oo.h | 2 + .../01-Basic/264-instantiation-integrity.ck | 30 ++++++ 6 files changed, 165 insertions(+), 14 deletions(-) create mode 100644 src/test/01-Basic/264-instantiation-integrity.ck diff --git a/src/core/chuck_emit.cpp b/src/core/chuck_emit.cpp index 303e111b4..4fa4e074a 100644 --- a/src/core/chuck_emit.cpp +++ b/src/core/chuck_emit.cpp @@ -5066,10 +5066,14 @@ t_CKBOOL emit_engine_instantiate_object( Chuck_Emitter * emit, Chuck_Type * type else if( !is_ref ) // not array { // emit object instantiation code, include pre constructor - emit->append( new Chuck_Instr_Instantiate_Object( type ) ); + emit->append( new Chuck_Instr_Instantiate_Object_Start( type ) ); // call pre constructor emit_engine_pre_constructor( emit, type, ctor_info ); + + // complete object instantiation | 1.5.4.3 (ge) added + // see Chuck_Instr_Instantiate_Object_Complete::execute() for explanation + emit->append( new Chuck_Instr_Instantiate_Object_Complete( type ) ); } return TRUE; diff --git a/src/core/chuck_instr.cpp b/src/core/chuck_instr.cpp index f535837eb..afca8ff51 100644 --- a/src/core/chuck_instr.cpp +++ b/src/core/chuck_instr.cpp @@ -4392,8 +4392,8 @@ const char * Chuck_Instr_Pre_Constructor::params() const //----------------------------------------------------------------------------- -// name: instantiate_object() -// desc: instantiate Object including data and virtual table +// name: initialize_object() +// desc: initialize Object including data and virtual table //----------------------------------------------------------------------------- t_CKBOOL initialize_object( Chuck_Object * object, Chuck_Type * type, Chuck_VM_Shred * shred, Chuck_VM * vm, t_CKBOOL setShredOrigin ) { @@ -4646,7 +4646,7 @@ Chuck_Object * instantiate_and_initialize_object( Chuck_Type * type, Chuck_VM_Sh // desc: instantiate a object, push its pointer on reg stack //----------------------------------------------------------------------------- inline void instantiate_object( Chuck_VM * vm, Chuck_VM_Shred * shred, - Chuck_Type * type ) + Chuck_Type * type, t_CKBOOL addRef ) { t_CKUINT *& reg_sp = (t_CKUINT *&)shred->reg->sp; @@ -4657,6 +4657,10 @@ inline void instantiate_object( Chuck_VM * vm, Chuck_VM_Shred * shred, // push the pointer on the operand stack push_( reg_sp, (t_CKUINT)object ); + // add reference? | 1.5.4.3 (ge) added this logic + // see Chuck_Instr_Instantiate_Object_Complete::execute() for explanation + if( addRef ) object->add_ref(); + // call preconstructor // call_pre_constructor( vm, shred, object, type, stack_offset ); @@ -4674,11 +4678,89 @@ inline void instantiate_object( Chuck_VM * vm, Chuck_VM_Shred * shred, //----------------------------------------------------------------------------- // name: execute() -// desc: instantiate object +// desc: instantiate a ChucK Object (starting step) +//----------------------------------------------------------------------------- +void Chuck_Instr_Instantiate_Object_Start::execute( Chuck_VM * vm, Chuck_VM_Shred * shred ) +{ + // instantiate a chuck Object + // 1.5.4.3 (ge) add (temporary) reference that will be decremented when + // instantiation complete; see Chuck_Instr_Instantiate_Object_Complete::execute() + // for explanation + instantiate_object( vm, shred, this->type, TRUE ); +} + + + + +//----------------------------------------------------------------------------- +// name: params() +// desc: ... +//----------------------------------------------------------------------------- +const char * Chuck_Instr_Instantiate_Object_Start::params() const +{ + static char buffer[CK_PRINT_BUF_LENGTH]; + snprintf( buffer, CK_PRINT_BUF_LENGTH, "%s", this->type->c_name() ); + return buffer; +} + + + + +//----------------------------------------------------------------------------- +// name: execute() +// desc: complete the process of instantiating an Object | 1.5.4.3 (ge) added //----------------------------------------------------------------------------- -void Chuck_Instr_Instantiate_Object::execute( Chuck_VM * vm, Chuck_VM_Shred * shred ) +void Chuck_Instr_Instantiate_Object_Complete::execute( Chuck_VM * vm, Chuck_VM_Shred * shred ) { - instantiate_object( vm, shred, this->type ); + // operand stack pointer + t_CKUINT *& reg_sp = (t_CKUINT *&)shred->reg->sp; + // get the object reference (should be top of operand stack) + Chuck_Object * obj = (Chuck_Object *)(*(reg_sp-1)); + // leave stack contents unchanged (no push or pop)... + + // NOTE: decrement (but NOT release) the temporary reference added by + // Chuck_Instr_Instantiate_Object_Start; this is done so that a constructor + // can work with `this` pointer with refcount > 0 BEFORE the + // instatiated object has a chance to be assigned to a variable (at which + // point it would have a proper reference) | 1.5.4.3 (ge) added + // + // NOTE: we are derementing only because of the possibility of + // the reference going back to 0, but we don't want to delete it at + // this point in case it's about to be assigned to a variable + // + // EXAMPLE: a situation that needs this: a constructor that calls a member + // function that returns itself: the statement cleanup logic could delete + // this object before the object can be assigned to a variable -- to account + // for the above, this mechanism temporarily boosts the instantiating + // object's refcount throughout Object instantiation + obj->dec_ref_no_release(); + + /*-------------------------------------------------------------------------- + | example -- also test/01-Basic/264-instantiation-integrity.ck + ---------------------------------------------------------------------------- + // a class definition + public class Foo + { + // a member function that returns `this` + fun Foo getFoo() { return this; } + // a constructor (takes argument for good measure) + fun Foo( vec3 sz ) + { + // return ourself -- note this is INSIDE Foo constructor + // which means it's before this instance can be assigned + // to a variable (as below); yet statements likes these + // have a clean-up logic that balances reference counts; + // so the refcount for Foo during instantiation must + // account for this interregnum; FYI this is typically + // handled by temporarily boosting the refcount during + // instantiation so it cannot be deleted from within + // its own constructor! + this.getFoo(); + } + } + // instantiate and assignment to variable + Foo foo( @(1,2,3) ); + --------------------------------------------------------------------------*/ } @@ -4688,7 +4770,7 @@ void Chuck_Instr_Instantiate_Object::execute( Chuck_VM * vm, Chuck_VM_Shred * sh // name: params() // desc: ... //----------------------------------------------------------------------------- -const char * Chuck_Instr_Instantiate_Object::params() const +const char * Chuck_Instr_Instantiate_Object_Complete::params() const { static char buffer[CK_PRINT_BUF_LENGTH]; snprintf( buffer, CK_PRINT_BUF_LENGTH, "%s", this->type->c_name() ); @@ -4726,7 +4808,7 @@ void Chuck_Instr_Pre_Ctor_Array_Top::execute( Chuck_VM * vm, Chuck_VM_Shred * sh else { // instantiate - instantiate_object( vm, shred, type ); + instantiate_object( vm, shred, type, FALSE ); } } @@ -6071,7 +6153,7 @@ t_CKBOOL Chuck_Instr_Stmt_Start::cleanupRefs( Chuck_VM_Shred * shred ) const char * Chuck_Instr_Stmt_Remember_Object::params() const { static char buffer[CK_PRINT_BUF_LENGTH]; - snprintf( buffer, CK_PRINT_BUF_LENGTH, "offset=%lu start=%p", (unsigned long)m_offset, m_stmtStart ); + snprintf( buffer, CK_PRINT_BUF_LENGTH, "offset=%lu start=%p addref:%s", (unsigned long)m_offset, m_stmtStart, m_addRef?"yes":"no" ); return buffer; } diff --git a/src/core/chuck_instr.h b/src/core/chuck_instr.h index 1b4ed0a01..36d3435d5 100644 --- a/src/core/chuck_instr.h +++ b/src/core/chuck_instr.h @@ -2945,13 +2945,33 @@ struct Chuck_Instr_Alloc_Word_Global : public Chuck_Instr_Unary_Op //----------------------------------------------------------------------------- -// name: struct Chuck_Instr_Instantiate_Object -// desc: instantiate object - leaves reference value on operand stack +// name: struct Chuck_Instr_Instantiate_Object_Start +// desc: instantiate object (starting step); leaves reference value on operand stack //----------------------------------------------------------------------------- -struct Chuck_Instr_Instantiate_Object : public Chuck_Instr +struct Chuck_Instr_Instantiate_Object_Start : public Chuck_Instr { public: - Chuck_Instr_Instantiate_Object( Chuck_Type * t ) + Chuck_Instr_Instantiate_Object_Start( Chuck_Type * t ) + { this->type = t; } + + virtual void execute( Chuck_VM * vm, Chuck_VM_Shred * shred ); + virtual const char * params() const; + +public: + Chuck_Type * type; +}; + + + + +//----------------------------------------------------------------------------- +// name: struct Chuck_Instr_Instantiate_Object_Complete | 1.5.4.3 (ge) added +// desc: instantiate object (completing step); leaves reference value on operand stack +//----------------------------------------------------------------------------- +struct Chuck_Instr_Instantiate_Object_Complete : public Chuck_Instr +{ +public: + Chuck_Instr_Instantiate_Object_Complete( Chuck_Type * t ) { this->type = t; } virtual void execute( Chuck_VM * vm, Chuck_VM_Shred * shred ); diff --git a/src/core/chuck_oo.cpp b/src/core/chuck_oo.cpp index 1b18790aa..a0bb1bd4d 100644 --- a/src/core/chuck_oo.cpp +++ b/src/core/chuck_oo.cpp @@ -112,6 +112,19 @@ void Chuck_VM_Object::add_ref() +//----------------------------------------------------------------------------- +// name: dec_ref_no_release() | 1.5.4.3 (ge) added +// desc: decrement reference only; no deletion +//----------------------------------------------------------------------------- +void Chuck_VM_Object::dec_ref_no_release() +{ + // decrement + if( m_ref_count > 0 ) m_ref_count--; +} + + + + //----------------------------------------------------------------------------- // name: release() // desc: decrement reference; deletes objects when refcount reaches 0; diff --git a/src/core/chuck_oo.h b/src/core/chuck_oo.h index 6e85cf91a..3f81267d9 100644 --- a/src/core/chuck_oo.h +++ b/src/core/chuck_oo.h @@ -81,6 +81,8 @@ struct Chuck_VM_Object virtual void add_ref(); // decrement reference; deletes objects when refcount reaches 0 virtual void release(); + // decrement reference only; no deletion | 1.5.4.3 (ge) added + virtual void dec_ref_no_release(); // lock virtual void lock(); // unlock | 1.5.0.0 (ge) added diff --git a/src/test/01-Basic/264-instantiation-integrity.ck b/src/test/01-Basic/264-instantiation-integrity.ck new file mode 100644 index 000000000..654ac3a18 --- /dev/null +++ b/src/test/01-Basic/264-instantiation-integrity.ck @@ -0,0 +1,30 @@ +// test object reference integrity during constructor +// THANKS to Miro Swisher for discovering this issue +// chuck-1.5.4.3 (ge) added Thanksgiving 2024 lol + +// a class definition +public class Foo +{ + // a member function that returns `this` + fun Foo getFoo() { return this; } + // a constructor (takes argument for good measure) + fun Foo( vec3 sz ) + { + // return ourself -- note this is INSIDE Foo constructor + // which means it's before this instance can be assigned + // to a variable (as below); yet statements likes these + // have a clean-up logic that balances reference counts; + // so the refcount for Foo during instantiation must + // account for this interregnum; FYI this is typically + // handled by temporarily boosting the refcount during + // instantiation so it cannot be deleted from within + // its own constructor! + this.getFoo(); + } +} + +// instantiate and assignment to variable +Foo foo( @(1,2,3) ); + +// no crash? ok +<<< "success" >>>;