From 130375b6d8e65be01ae3dbb70de969c6e24db64b Mon Sep 17 00:00:00 2001 From: Wayward Heart <91356680+WaywardHeart@users.noreply.github.com> Date: Mon, 17 Jun 2024 11:49:56 -0500 Subject: [PATCH] Properly handle uninitialized and reinitialized objects In RGSS, uninitialized disposable objects (and Fonts, sort of) are technically disposed, and other objects (Tone, Color, Rect, and Table) are created with all values set to 0. It's also possible to reinitialize them, although reinitializing disposables leaks memory. This commit causes MiniFFI and FileInt objects to improperly raise disposed errors if used while uninitialized, but that feels acceptable to me. --- binding/binding-util.h | 57 ++++++++++++++++++++++++++++++++--- binding/bitmap-binding.cpp | 55 ++++++++++++++++++++++++++++----- binding/disposable-binding.h | 6 ++-- binding/etc-binding.cpp | 30 +++++++----------- binding/font-binding.cpp | 10 +++++- binding/table-binding.cpp | 26 +++++++++++----- binding/tilemap-binding.cpp | 13 ++++++-- binding/tilemapvx-binding.cpp | 13 ++++++-- 8 files changed, 164 insertions(+), 46 deletions(-) diff --git a/binding/binding-util.h b/binding/binding-util.h index 53a2ad72..4839e987 100644 --- a/binding/binding-util.h +++ b/binding/binding-util.h @@ -198,20 +198,50 @@ template static VALUE classAllocate(VALUE klass) { } #endif +#if RAPI_FULL > 187 +#define CLASS_ALLOCATE_PRE_INIT(Name, initializeFunc) \ +static VALUE Name##AllocatePreInit(VALUE klass) { \ + VALUE ret = classAllocate<& Name##Type>(klass); \ + \ + initializeFunc(0, 0, ret); \ + \ + return ret; \ +} +#else +#define CLASS_ALLOCATE_PRE_INIT(Name, initializeFunc) \ +static VALUE Name##AllocatePreInit(VALUE klass) { \ + VALUE ret = Name##Allocate(klass); \ + \ + initializeFunc(0, 0, ret); \ + \ + return ret; \ +} +#endif + template static void freeInstance(void *inst) { delete static_cast(inst); } void raiseDisposedAccess(VALUE self); -template inline C *getPrivateData(VALUE self) { +template inline C *getPrivateDataNoRaise(VALUE self) { #if RAPI_FULL > 187 - C *c = static_cast(RTYPEDDATA_DATA(self)); + return static_cast(RTYPEDDATA_DATA(self)); #else - C *c = static_cast(DATA_PTR(self)); + return static_cast(DATA_PTR(self)); #endif +} + +template inline C *getPrivateData(VALUE self) { + C *c = getPrivateDataNoRaise(self); + if (!c) { - raiseRbExc(Exception(Exception::MKXPError, "No instance data for variable (missing call to super?)")); + //raiseRbExc(Exception(Exception::MKXPError, "No instance data for variable (missing call to super?)")); + + /* FIXME: MiniFFI and FileInt don't have default allocations + * despite not being disposables. Should they be fixed, + * or just left with a misleading error message? */ + raiseDisposedAccess(self); } return c; } @@ -246,9 +276,28 @@ getPrivateDataCheck(VALUE self, const char *type) } static inline void setPrivateData(VALUE self, void *p) { + /* RGSS's behavior is to just leak memory if a disposable is reinitialized, + * with the original disposable being left permanently instantiated, + * but that's (1) bad, and (2) would currently cause memory access issues + * when things like a sprite's src_rect inevitably get GC'd, so we're not + * copying that. */ #if RAPI_FULL > 187 + // Free the old value if it already exists (initialize called twice?) + if (RTYPEDDATA_DATA(self) && (RTYPEDDATA_DATA(self) != p)) { + /* RUBY_TYPED_NEVER_FREE == 0, and we don't use + * RUBY_TYPED_DEFAULT_FREE for our stuff, so just + * checking if it's truthy should be fine */ + if (RTYPEDDATA_TYPE(self)->function.dfree) + (*RTYPEDDATA_TYPE(self)->function.dfree)(RTYPEDDATA_DATA(self)); + } RTYPEDDATA_DATA(self) = p; #else + // Free the old value if it already exists (initialize called twice?) + if (DATA_PTR(self) && (DATA_PTR(self) != p)) { + /* As above, just check if it's truthy */ + if (RDATA(self)->dfree) + (*RDATA(self)->dfree)(DATA_PTR(self)); + } DATA_PTR(self) = p; #endif } diff --git a/binding/bitmap-binding.cpp b/binding/bitmap-binding.cpp index 0b1b51a9..b38614e5 100644 --- a/binding/bitmap-binding.cpp +++ b/binding/bitmap-binding.cpp @@ -141,9 +141,11 @@ RB_METHOD_GUARD(bitmapBlt) { &opacity RB_ARG_END); src = getPrivateDataCheck(srcObj, BitmapType); - srcRect = getPrivateDataCheck(srcRectObj, RectType); + if (src) { + srcRect = getPrivateDataCheck(srcRectObj, RectType); - GFX_GUARD_EXC(b->blt(x, y, *src, srcRect->toIntRect(), opacity);); + GFX_GUARD_EXC(b->blt(x, y, *src, srcRect->toIntRect(), opacity);); + } return self; } @@ -164,11 +166,13 @@ RB_METHOD_GUARD(bitmapStretchBlt) { &opacity RB_ARG_END); src = getPrivateDataCheck(srcObj, BitmapType); - destRect = getPrivateDataCheck(destRectObj, RectType); - srcRect = getPrivateDataCheck(srcRectObj, RectType); - - GFX_GUARD_EXC(b->stretchBlt(destRect->toIntRect(), *src, srcRect->toIntRect(), - opacity);); + if (src) { + destRect = getPrivateDataCheck(destRectObj, RectType); + srcRect = getPrivateDataCheck(srcRectObj, RectType); + + GFX_GUARD_EXC(b->stretchBlt(destRect->toIntRect(), *src, srcRect->toIntRect(), + opacity);); + } return self; } @@ -332,7 +336,40 @@ RB_METHOD_GUARD(bitmapTextSize) { } RB_METHOD_GUARD_END -DEF_GFX_PROP_OBJ_VAL(Bitmap, Font, Font, "font") +RB_METHOD(BitmapGetFont) { + RB_UNUSED_PARAM; + checkDisposed(self); + return rb_iv_get(self, "font"); +} +RB_METHOD_GUARD(BitmapSetFont) { + rb_check_argc(argc, 1); + Bitmap *b = getPrivateData(self); + VALUE propObj = *argv; + + Font *prop = getPrivateDataCheck(propObj, FontType); + if (prop) { + GFX_GUARD_EXC(b->setFont(*prop);) + + VALUE f = rb_iv_get(self, "font"); + if (f) { + rb_iv_set(f, "name", rb_iv_get(propObj, "name")); + rb_iv_set(f, "size", rb_iv_get(propObj, "size")); + rb_iv_set(f, "bold", rb_iv_get(propObj, "bold")); + rb_iv_set(f, "italic", rb_iv_get(propObj, "italic")); + + if (rgssVer >= 2) { + rb_iv_set(f, "shadow", rb_iv_get(propObj, "shadow")); + } + + if (rgssVer >= 3) { + rb_iv_set(f, "outline", rb_iv_get(propObj, "outline")); + } + } + } + + return propObj; +} +RB_METHOD_GUARD_END RB_METHOD_GUARD(bitmapGradientFillRect) { Bitmap *b = getPrivateData(self); @@ -602,6 +639,8 @@ RB_METHOD_GUARD(bitmapAddFrame){ rb_scan_args(argc, argv, "11", &srcBitmap, &position); Bitmap *src = getPrivateDataCheck(srcBitmap, BitmapType); + if (!src) + raiseDisposedAccess(srcBitmap); Bitmap *b = getPrivateData(self); diff --git a/binding/disposable-binding.h b/binding/disposable-binding.h index 1f531991..69fac862 100644 --- a/binding/disposable-binding.h +++ b/binding/disposable-binding.h @@ -30,8 +30,8 @@ template RB_METHOD(disposableDispose) { RB_UNUSED_PARAM; - - C *d = getPrivateData(self); + + C *d = getPrivateDataNoRaise(self); if (!d) return Qnil; @@ -46,7 +46,7 @@ RB_METHOD(disposableIsDisposed) { RB_UNUSED_PARAM; - C *d = getPrivateData(self); + C *d = getPrivateDataNoRaise(self); if (!d) return Qtrue; diff --git a/binding/etc-binding.cpp b/binding/etc-binding.cpp index ba4458a8..130bbc3e 100644 --- a/binding/etc-binding.cpp +++ b/binding/etc-binding.cpp @@ -107,7 +107,13 @@ EQUAL_FUN(Rect) rb_get_args(argc, argv, param_t_s, &p1, &p2, &p3, &p4 RB_ARG_END); \ k = new Klass(p1, p2, p3, p4); \ } \ + Klass *orig = getPrivateDataNoRaise(self); \ + if (orig) { \ + *orig = *k; \ + delete k; \ + } else { \ setPrivateData(self, k); \ + } \ return self; \ } @@ -208,11 +214,14 @@ INITCOPY_FUN(Tone) INITCOPY_FUN(Color) INITCOPY_FUN(Rect) -#if RAPI_FULL > 187 +CLASS_ALLOCATE_PRE_INIT(Tone, ToneInitialize); +CLASS_ALLOCATE_PRE_INIT(Color, ColorInitialize); +CLASS_ALLOCATE_PRE_INIT(Rect, RectInitialize); + #define INIT_BIND(Klass) \ { \ klass = rb_define_class(#Klass, rb_cObject); \ - rb_define_alloc_func(klass, classAllocate<&Klass##Type>); \ + rb_define_alloc_func(klass, Klass##AllocatePreInit); \ rb_define_class_method(klass, "_load", Klass##Load); \ serializableBindingInit(klass); \ _rb_define_method(klass, "initialize", Klass##Initialize); \ @@ -224,23 +233,6 @@ INITCOPY_FUN(Rect) _rb_define_method(klass, "to_s", Klass##Stringify); \ _rb_define_method(klass, "inspect", Klass##Stringify); \ } -#else -#define INIT_BIND(Klass) \ - { \ - klass = rb_define_class(#Klass, rb_cObject); \ - rb_define_alloc_func(klass, Klass##Allocate); \ - rb_define_class_method(klass, "_load", Klass##Load); \ - serializableBindingInit(klass); \ - _rb_define_method(klass, "initialize", Klass##Initialize); \ - _rb_define_method(klass, "initialize_copy", Klass##InitializeCopy); \ - _rb_define_method(klass, "set", Klass##Set); \ - _rb_define_method(klass, "==", Klass##Equal); \ - _rb_define_method(klass, "===", Klass##Equal); \ - _rb_define_method(klass, "eql?", Klass##Equal); \ - _rb_define_method(klass, "to_s", Klass##Stringify); \ - _rb_define_method(klass, "inspect", Klass##Stringify); \ - } -#endif #define MRB_ATTR_R(Class, attr) \ mrb_define_method(mrb, klass, #attr, Class##Get_##attr, MRB_ARGS_NONE()) diff --git a/binding/font-binding.cpp b/binding/font-binding.cpp index 664806a6..6c944940 100644 --- a/binding/font-binding.cpp +++ b/binding/font-binding.cpp @@ -88,7 +88,15 @@ RB_METHOD(fontInitialize) { * However the same bug/behavior exists in all RM versions. */ rb_iv_set(self, "name", namesObj); - setPrivateData(self, f); + Font *orig = getPrivateDataNoRaise(self); + if (orig) + { + *orig = *f; + delete f; + f = orig; + } else { + setPrivateData(self, f); + } /* Wrap property objects */ f->initDynAttribs(); diff --git a/binding/table-binding.cpp b/binding/table-binding.cpp index 2ef51433..770514a3 100644 --- a/binding/table-binding.cpp +++ b/binding/table-binding.cpp @@ -57,9 +57,14 @@ RB_METHOD(tableInitialize) { parseArgsTableSizes(argc, argv, &x, &y, &z); - Table *t = new Table(x, y, z); + Table *t = getPrivateDataNoRaise(self); + if (t) { + t->resize(x, y, z); + } else { + t = new Table(x, y, z); - setPrivateData(self, t); + setPrivateData(self, t); + } return self; } @@ -152,13 +157,20 @@ RB_METHOD_GUARD_END MARSH_LOAD_FUN(Table) INITCOPY_FUN(Table) + +RB_METHOD(tableInitializeDefault) { + Table *t = new Table(0, 0, 0); + + setPrivateData(self, t); + + return self; +} + +CLASS_ALLOCATE_PRE_INIT(Table, tableInitializeDefault); + void tableBindingInit() { VALUE klass = rb_define_class("Table", rb_cObject); -#if RAPI_FULL > 187 - rb_define_alloc_func(klass, classAllocate<&TableType>); -#else - rb_define_alloc_func(klass, TableAllocate); -#endif + rb_define_alloc_func(klass, TableAllocatePreInit); serializableBindingInit
(klass); diff --git a/binding/tilemap-binding.cpp b/binding/tilemap-binding.cpp index d2c5a754..79d08b34 100644 --- a/binding/tilemap-binding.cpp +++ b/binding/tilemap-binding.cpp @@ -35,7 +35,10 @@ DEF_TYPE_CUSTOMFREE(TilemapAutotiles, RUBY_TYPED_NEVER_FREE); #endif RB_METHOD(tilemapAutotilesSet) { - Tilemap::Autotiles *a = getPrivateData(self); + Tilemap::Autotiles *a = getPrivateDataNoRaise(self); + + if (!a) + return self; int i; VALUE bitmapObj; @@ -93,12 +96,18 @@ RB_METHOD(tilemapInitialize) { t->initDynAttribs(); + /* Dispose the old autotiles if we're reinitializing. + * See the comment in setPrivateData for more info. */ + VALUE autotilesObj = rb_iv_get(self, "autotiles"); + if (autotilesObj != Qnil) + setPrivateData(autotilesObj, 0); + wrapProperty(self, &t->getAutotiles(), "autotiles", TilemapAutotilesType); wrapProperty(self, &t->getColor(), "color", ColorType); wrapProperty(self, &t->getTone(), "tone", ToneType); - VALUE autotilesObj = rb_iv_get(self, "autotiles"); + autotilesObj = rb_iv_get(self, "autotiles"); VALUE ary = rb_ary_new2(7); for (int i = 0; i < 7; ++i) diff --git a/binding/tilemapvx-binding.cpp b/binding/tilemapvx-binding.cpp index 3e616571..992093c9 100644 --- a/binding/tilemapvx-binding.cpp +++ b/binding/tilemapvx-binding.cpp @@ -58,10 +58,16 @@ RB_METHOD(tilemapVXInitialize) { rb_iv_set(self, "viewport", viewportObj); + /* Dispose the old bitmap array if we're reinitializing. + * See the comment in setPrivateData for more info. */ + VALUE autotilesObj = rb_iv_get(self, "bitmap_array"); + if (autotilesObj != Qnil) + setPrivateData(autotilesObj, 0); + wrapProperty(self, &t->getBitmapArray(), "bitmap_array", BitmapArrayType, rb_const_get(rb_cObject, rb_intern("Tilemap"))); - VALUE autotilesObj = rb_iv_get(self, "bitmap_array"); + autotilesObj = rb_iv_get(self, "bitmap_array"); VALUE ary = rb_ary_new2(9); for (int i = 0; i < 9; ++i) @@ -106,7 +112,10 @@ DEF_GFX_PROP_I(TilemapVX, OX) DEF_GFX_PROP_I(TilemapVX, OY) RB_METHOD(tilemapVXBitmapsSet) { - TilemapVX::BitmapArray *a = getPrivateData(self); + TilemapVX::BitmapArray *a = getPrivateDataNoRaise(self); + + if (!a) + return self; int i; VALUE bitmapObj;