Properly handle uninitialized and reinitialized objects

WaywardHeart · WaywardHeart · commit 130375b6d8e6 · 2024-08-02T09:26:51.000-05:00
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.
diff --git a/binding/binding-util.h b/binding/binding-util.h
@@ -198,20 +198,50 @@ template <rb_data_type_t *rbType> 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 <class C> static void freeInstance(void *inst) {
     delete static_cast<C *>(inst);
 }
 
 void raiseDisposedAccess(VALUE self);
 
-template <class C> inline C *getPrivateData(VALUE self) {
+template <class C> inline C *getPrivateDataNoRaise(VALUE self) {
 #if RAPI_FULL > 187
-    C *c = static_cast<C *>(RTYPEDDATA_DATA(self));
+    return static_cast<C *>(RTYPEDDATA_DATA(self));
 #else
-    C *c = static_cast<C *>(DATA_PTR(self));
+    return static_cast<C *>(DATA_PTR(self));
 #endif
+}
+
+template <class C> inline C *getPrivateData(VALUE self) {
+    C *c = getPrivateDataNoRaise<C>(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
@@ -141,9 +141,11 @@ RB_METHOD_GUARD(bitmapBlt) {
                 &opacity RB_ARG_END);
     
     src = getPrivateDataCheck<Bitmap>(srcObj, BitmapType);
-    srcRect = getPrivateDataCheck<Rect>(srcRectObj, RectType);
+    if (src) {
+        srcRect = getPrivateDataCheck<Rect>(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<Bitmap>(srcObj, BitmapType);
-    destRect = getPrivateDataCheck<Rect>(destRectObj, RectType);
-    srcRect = getPrivateDataCheck<Rect>(srcRectObj, RectType);
-    
-    GFX_GUARD_EXC(b->stretchBlt(destRect->toIntRect(), *src, srcRect->toIntRect(),
-                            opacity););
+    if (src) {
+        destRect = getPrivateDataCheck<Rect>(destRectObj, RectType);
+        srcRect = getPrivateDataCheck<Rect>(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<Bitmap>(self);
+    return rb_iv_get(self, "font");
+}
+RB_METHOD_GUARD(BitmapSetFont) {
+    rb_check_argc(argc, 1);
+    Bitmap *b = getPrivateData<Bitmap>(self);
+    VALUE propObj = *argv;
+    
+    Font *prop = getPrivateDataCheck<Font>(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<Bitmap>(self);
@@ -602,6 +639,8 @@ RB_METHOD_GUARD(bitmapAddFrame){
     rb_scan_args(argc, argv, "11", &srcBitmap, &position);
     
     Bitmap *src = getPrivateDataCheck<Bitmap>(srcBitmap, BitmapType);
+    if (!src)
+        raiseDisposedAccess(srcBitmap);
     
     Bitmap *b = getPrivateData<Bitmap>(self);
     
diff --git a/binding/disposable-binding.h b/binding/disposable-binding.h
@@ -30,8 +30,8 @@ template<class C>
 RB_METHOD(disposableDispose)
 {
 	RB_UNUSED_PARAM;
-
-	C *d = getPrivateData<C>(self);
+	
+	C *d = getPrivateDataNoRaise<C>(self);
 
 	if (!d)
 		return Qnil;
@@ -46,7 +46,7 @@ RB_METHOD(disposableIsDisposed)
 {
 	RB_UNUSED_PARAM;
 
-	C *d = getPrivateData<C>(self);
+	C *d = getPrivateDataNoRaise<C>(self);
 
 	if (!d)
 		return Qtrue;
diff --git 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<Klass>(self);                          \
+    if (orig) {                                                                \
+      *orig = *k;                                                              \
+      delete k;                                                                \
+    } else {                                                                   \
     setPrivateData(self, k);                                                   \
+    }                                                                          \
     return self;                                                               \
   }
 
@@ -208,27 +214,14 @@ INITCOPY_FUN(Tone)
 INITCOPY_FUN(Color)
 INITCOPY_FUN(Rect)
 
-#if RAPI_FULL > 187
-#define INIT_BIND(Klass)                                                       \
-  {                                                                            \
-    klass = rb_define_class(#Klass, rb_cObject);                               \
-    rb_define_alloc_func(klass, classAllocate<&Klass##Type>);                  \
-    rb_define_class_method(klass, "_load", Klass##Load);                       \
-    serializableBindingInit<Klass>(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);                     \
-  }
-#else
+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, Klass##Allocate);                              \
+    rb_define_alloc_func(klass, Klass##AllocatePreInit);                       \
     rb_define_class_method(klass, "_load", Klass##Load);                       \
     serializableBindingInit<Klass>(klass);                                     \
     _rb_define_method(klass, "initialize", Klass##Initialize);                 \
@@ -240,7 +233,6 @@ INITCOPY_FUN(Rect)
     _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
@@ -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<Font>(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
@@ -57,9 +57,14 @@ RB_METHOD(tableInitialize) {
 
   parseArgsTableSizes(argc, argv, &x, &y, &z);
 
-  Table *t = new Table(x, y, z);
+  Table *t = getPrivateDataNoRaise<Table>(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<Table>(klass);
 
diff --git 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<Tilemap::Autotiles>(self);
+    Tilemap::Autotiles *a = getPrivateDataNoRaise<Tilemap::Autotiles>(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
@@ -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<TilemapVX::BitmapArray>(self);
+    TilemapVX::BitmapArray *a = getPrivateDataNoRaise<TilemapVX::BitmapArray>(self);
+    
+    if (!a)
+        return self;
     
     int i;
     VALUE bitmapObj;
