From 4ff563725b420590c48b19354e245fcae2905105 Mon Sep 17 00:00:00 2001 From: Jonas Kulla Date: Fri, 27 Sep 2013 04:49:48 +0200 Subject: [PATCH] Make 'rb_get_args()' va_arg passing safer by introducing a termination marker What can I say. I made a pact with the devil, and paid dearly. Almost a whole day's worth of debugging, actually. Not again. If this turns out to be slow we can always optimize the critical parts (with no variable param count) later, or completely remove it. --- binding-mri/audio-binding.cpp | 4 +-- binding-mri/binding-util.cpp | 41 +++++++++++++++++++++++++++ binding-mri/binding-util.h | 13 +++++---- binding-mri/bitmap-binding.cpp | 24 ++++++++-------- binding-mri/etc-binding.cpp | 8 +++--- binding-mri/filesystem-binding.cpp | 6 ++-- binding-mri/flashable-binding.h | 2 +- binding-mri/font-binding.cpp | 12 ++++---- binding-mri/graphics-binding.cpp | 6 ++-- binding-mri/input-binding.cpp | 6 ++-- binding-mri/sceneelement-binding.h | 4 +-- binding-mri/table-binding.cpp | 2 +- binding-mri/tilemap-binding.cpp | 6 ++-- binding-mri/viewport-binding.cpp | 4 +-- binding-mri/viewportelement-binding.h | 2 +- 15 files changed, 92 insertions(+), 48 deletions(-) diff --git a/binding-mri/audio-binding.cpp b/binding-mri/audio-binding.cpp index 4fa35bb..5f57768 100644 --- a/binding-mri/audio-binding.cpp +++ b/binding-mri/audio-binding.cpp @@ -31,7 +31,7 @@ const char *filename; \ int volume = 100; \ int pitch = 100; \ - rb_get_args(argc, argv, "z|ii", &filename, &volume, &pitch); \ + rb_get_args(argc, argv, "z|ii", &filename, &volume, &pitch, RB_ARG_END); \ GUARD_EXC( gState->audio().entity##Play(filename, volume, pitch); ) \ return Qnil; \ } \ @@ -47,7 +47,7 @@ RB_METHOD(audio_##entity##Fade) \ { \ RB_UNUSED_PARAM; \ int time; \ - rb_get_args(argc, argv, "i", &time); \ + rb_get_args(argc, argv, "i", &time, RB_ARG_END); \ gState->audio().bgmFade(time); \ return Qnil; \ } diff --git a/binding-mri/binding-util.cpp b/binding-mri/binding-util.cpp index cdd1d95..f249034 100644 --- a/binding-mri/binding-util.cpp +++ b/binding-mri/binding-util.cpp @@ -282,10 +282,51 @@ rb_get_args(int argc, VALUE *argv, const char *format, ...) } } + /* Pop remaining arg pointers off + * the stack to check for RB_ARG_END */ + format--; + + while ((c = *format++)) + { + switch (c) + { + case 'o' : + case 'S' : + va_arg(ap, VALUE*); + break; + + case 's' : + va_arg(ap, const char**); + va_arg(ap, int*); + break; + + case 'z' : + va_arg(ap, const char**); + break; + + case 'f' : + va_arg(ap, double*); + break; + + case 'i' : + va_arg(ap, int*); + break; + + case 'b' : + va_arg(ap, bool*); + break; + } + } + // FIXME print num of needed args vs provided if (!c && argc > argI) rb_raise(rb_eArgError, "wrong number of arguments"); + /* Verify correct termination */ + void *argEnd = va_arg(ap, void*); + Q_UNUSED(argEnd); + Q_ASSERT(argEnd == RB_ARG_END); + va_end(ap); return argI; diff --git a/binding-mri/binding-util.h b/binding-mri/binding-util.h index 0adcd4b..9a6b3d9 100644 --- a/binding-mri/binding-util.h +++ b/binding-mri/binding-util.h @@ -69,7 +69,7 @@ void initType(rb_data_type_struct &type, void (*freeInst)(void*)); template -static inline void freeInstance(void *inst) +static void freeInstance(void *inst) { delete static_cast(inst); } @@ -132,6 +132,9 @@ wrapNilProperty(VALUE self, const char *iv) int rb_get_args(int argc, VALUE *argv, const char *format, ...); +/* Always terminate 'rb_get_args' with this */ +#define RB_ARG_END ((void*) -1) + typedef VALUE (*RubyMethod)(int argc, VALUE *argv, VALUE self); static inline void @@ -158,7 +161,7 @@ objectLoad(int argc, VALUE *argv, VALUE self, rb_data_type_struct &type) { const char *data; int dataLen; - rb_get_args(argc, argv, "s", &data, &dataLen); + rb_get_args(argc, argv, "s", &data, &dataLen, RB_ARG_END); VALUE obj = rb_obj_alloc(self); @@ -227,7 +230,7 @@ rb_bool_new(bool value) Klass *k = getPrivateData(self); \ VALUE propObj; \ PropKlass *prop; \ - rb_get_args(argc, argv, "o", &propObj); \ + rb_get_args(argc, argv, "o", &propObj, RB_ARG_END); \ prop = getPrivateDataCheck(propObj, PropKlass##Type); \ GUARD_EXC( k->set##PropName(prop); ) \ rb_iv_set(self, prop_iv, propObj); \ @@ -249,7 +252,7 @@ rb_bool_new(bool value) Klass *k = getPrivateData(self); \ VALUE propObj; \ PropKlass *prop; \ - rb_get_args(argc, argv, "o", &propObj); \ + rb_get_args(argc, argv, "o", &propObj, RB_ARG_END); \ if (rb_type(propObj) == RUBY_T_NIL) \ prop = 0; \ else \ @@ -271,7 +274,7 @@ rb_bool_new(bool value) { \ Klass *k = getPrivateData(self); \ type value; \ - rb_get_args(argc, argv, param_t_s, &value); \ + rb_get_args(argc, argv, param_t_s, &value, RB_ARG_END); \ GUARD_EXC( k->set##PropName(value); ) \ return value_fun(value); \ } diff --git a/binding-mri/bitmap-binding.cpp b/binding-mri/bitmap-binding.cpp index b864f3b..7243608 100644 --- a/binding-mri/bitmap-binding.cpp +++ b/binding-mri/bitmap-binding.cpp @@ -39,14 +39,14 @@ RB_METHOD(bitmapInitialize) if (argc == 1) { char *filename; - rb_get_args(argc, argv, "z", &filename); + rb_get_args(argc, argv, "z", &filename, RB_ARG_END); GUARD_EXC( b = new Bitmap(filename); ) } else { int width, height; - rb_get_args(argc, argv, "ii", &width, &height); + rb_get_args(argc, argv, "ii", &width, &height, RB_ARG_END); GUARD_EXC( b = new Bitmap(width, height); ) } @@ -114,7 +114,7 @@ RB_METHOD(bitmapBlt) Bitmap *src; Rect *srcRect; - rb_get_args(argc, argv, "iioo|i", &x, &y, &srcObj, &srcRectObj, &opacity); + rb_get_args(argc, argv, "iioo|i", &x, &y, &srcObj, &srcRectObj, &opacity, RB_ARG_END); src = getPrivateDataCheck(srcObj, BitmapType); srcRect = getPrivateDataCheck(srcRectObj, RectType); @@ -136,7 +136,7 @@ RB_METHOD(bitmapStretchBlt) Bitmap *src; Rect *destRect, *srcRect; - rb_get_args(argc, argv, "ooo|i", &destRectObj, &srcObj, &srcRectObj); + rb_get_args(argc, argv, "ooo|i", &destRectObj, &srcObj, &srcRectObj, &opacity, RB_ARG_END); src = getPrivateDataCheck(srcObj, BitmapType); destRect = getPrivateDataCheck(destRectObj, RectType); @@ -159,7 +159,7 @@ RB_METHOD(bitmapFillRect) VALUE rectObj; Rect *rect; - rb_get_args(argc, argv, "oo", &rectObj, &colorObj); + rb_get_args(argc, argv, "oo", &rectObj, &colorObj, RB_ARG_END); rect = getPrivateDataCheck(rectObj, RectType); color = getPrivateDataCheck(colorObj, ColorType); @@ -170,7 +170,7 @@ RB_METHOD(bitmapFillRect) { int x, y, width, height; - rb_get_args(argc, argv, "iiiio", &x, &y, &width, &height, &colorObj); + rb_get_args(argc, argv, "iiiio", &x, &y, &width, &height, &colorObj, RB_ARG_END); color = getPrivateDataCheck(colorObj, ColorType); @@ -197,7 +197,7 @@ RB_METHOD(bitmapGetPixel) int x, y; - rb_get_args(argc, argv, "ii", &x, &y); + rb_get_args(argc, argv, "ii", &x, &y, RB_ARG_END); GUARD_EXC( if (x < 0 || y < 0 || x >= b->width() || y >= b->height()) @@ -221,7 +221,7 @@ RB_METHOD(bitmapSetPixel) Color *color; - rb_get_args(argc, argv, "iio", &x, &y, &colorObj); + rb_get_args(argc, argv, "iio", &x, &y, &colorObj, RB_ARG_END); color = getPrivateDataCheck(colorObj, ColorType); @@ -236,7 +236,7 @@ RB_METHOD(bitmapHueChange) int hue; - rb_get_args(argc, argv, "i", &hue); + rb_get_args(argc, argv, "i", &hue, RB_ARG_END); GUARD_EXC( b->hueChange(hue); ); @@ -255,7 +255,7 @@ RB_METHOD(bitmapDrawText) VALUE rectObj; Rect *rect; - rb_get_args(argc, argv, "oz|i", &rectObj, &str, &align); + rb_get_args(argc, argv, "oz|i", &rectObj, &str, &align, RB_ARG_END); rect = getPrivateDataCheck(rectObj, RectType); @@ -265,7 +265,7 @@ RB_METHOD(bitmapDrawText) { int x, y, width, height; - rb_get_args(argc, argv, "iiiiz|i", &x, &y, &width, &height, &str, &align); + rb_get_args(argc, argv, "iiiiz|i", &x, &y, &width, &height, &str, &align, RB_ARG_END); GUARD_EXC( b->drawText(x, y, width, height, str, align); ); } @@ -279,7 +279,7 @@ RB_METHOD(bitmapTextSize) const char *str; - rb_get_args(argc, argv, "z", &str); + rb_get_args(argc, argv, "z", &str, RB_ARG_END); IntRect value; GUARD_EXC( value = b->textSize(str); ); diff --git a/binding-mri/etc-binding.cpp b/binding-mri/etc-binding.cpp index 1e83527..f57e3c7 100644 --- a/binding-mri/etc-binding.cpp +++ b/binding-mri/etc-binding.cpp @@ -40,7 +40,7 @@ DEF_TYPE(Rect); { \ Klass *p = getPrivateData(self); \ arg_type arg; \ - rb_get_args(argc, argv, arg_t_s, &arg); \ + rb_get_args(argc, argv, arg_t_s, &arg, RB_ARG_END); \ p->set##Attr(arg); \ return *argv; \ } @@ -69,7 +69,7 @@ ATTR_INT_RW(Rect, Height) Klass *p = getPrivateData(self); \ VALUE otherObj; \ Klass *other; \ - rb_get_args(argc, argv, "o", &otherObj); \ + rb_get_args(argc, argv, "o", &otherObj, RB_ARG_END); \ other = getPrivateDataCheck(otherObj, Klass##Type); \ return rb_bool_new(*p == *other); \ } @@ -82,7 +82,7 @@ EQUAL_FUN(Rect) RB_METHOD(Klass##Initialize) \ { \ param_type p1, p2, p3, p4 = last_param_def; \ - rb_get_args(argc, argv, param_t_s, &p1, &p2, &p3, &p4); \ + rb_get_args(argc, argv, param_t_s, &p1, &p2, &p3, &p4, RB_ARG_END); \ Klass *k = new Klass(p1, p2, p3, p4); \ setPrivateData(self, k, Klass##Type); \ return self; \ @@ -96,7 +96,7 @@ INIT_FUN(Rect, int, "iiii", 0) RB_METHOD(Klass##Set) \ { \ param_type p1, p2, p3, p4 = last_param_def; \ - rb_get_args(argc, argv, param_t_s, &p1, &p2, &p3, &p4); \ + rb_get_args(argc, argv, param_t_s, &p1, &p2, &p3, &p4, RB_ARG_END); \ Klass *k = getPrivateData(self); \ k->set(p1, p2, p3, p4); \ return self; \ diff --git a/binding-mri/filesystem-binding.cpp b/binding-mri/filesystem-binding.cpp index 99ca073..f463ad9 100644 --- a/binding-mri/filesystem-binding.cpp +++ b/binding-mri/filesystem-binding.cpp @@ -47,7 +47,7 @@ RB_METHOD(fileIntRead) { int length = -1; - rb_get_args(argc, argv, "i", &length); + rb_get_args(argc, argv, "i", &length, RB_ARG_END); SDL_RWops *ops = getPrivateData(self); @@ -129,7 +129,7 @@ RB_METHOD(kernelLoadData) RB_UNUSED_PARAM; const char *filename; - rb_get_args(argc, argv, "z", &filename); + rb_get_args(argc, argv, "z", &filename, RB_ARG_END); return kernelLoadDataInt(filename); } @@ -141,7 +141,7 @@ RB_METHOD(kernelSaveData) VALUE obj; VALUE filename; - rb_get_args(argc, argv, "oS", &obj, &filename); + rb_get_args(argc, argv, "oS", &obj, &filename, RB_ARG_END); VALUE file = rb_funcall(rb_cFile, rb_intern("open"), 2, filename, rb_str_new_cstr("w")); diff --git a/binding-mri/flashable-binding.h b/binding-mri/flashable-binding.h index a78b0cd..26f7adc 100644 --- a/binding-mri/flashable-binding.h +++ b/binding-mri/flashable-binding.h @@ -36,7 +36,7 @@ RB_METHOD(flashableFlash) Color *color; - rb_get_args(argc, argv, "oi", &colorObj, &duration); + rb_get_args(argc, argv, "oi", &colorObj, &duration, RB_ARG_END); if (rb_type(colorObj) == RUBY_T_NIL) { diff --git a/binding-mri/font-binding.cpp b/binding-mri/font-binding.cpp index d4114b8..2db688d 100644 --- a/binding-mri/font-binding.cpp +++ b/binding-mri/font-binding.cpp @@ -31,7 +31,7 @@ RB_METHOD(fontDoesExist) RB_UNUSED_PARAM; const char *name; - rb_get_args(argc, argv, "z", &name); + rb_get_args(argc, argv, "z", &name, RB_ARG_END); return rb_bool_new(Font::doesExist(name)); } @@ -41,7 +41,7 @@ RB_METHOD(fontInitialize) const char *name = 0; int size = 0; - rb_get_args(argc, argv, "|zi", &name, &size); + rb_get_args(argc, argv, "|zi", &name, &size, RB_ARG_END); Font *f = new Font(name, size); @@ -68,7 +68,7 @@ RB_METHOD(FontSetName) Font *f = getPrivateData(self); VALUE name; - rb_get_args(argc, argv, "S", &name); + rb_get_args(argc, argv, "S", &name, RB_ARG_END); f->setName(RSTRING_PTR(name)); @@ -93,7 +93,7 @@ DEF_PROP_OBJ(Font, Color, Color, "color") { \ RB_UNUSED_PARAM; \ type value; \ - rb_get_args(argc, argv, param_t_s, &value); \ + rb_get_args(argc, argv, param_t_s, &value, RB_ARG_END); \ Klass::set##PropName(value); \ return value_fun(value); \ } @@ -112,7 +112,7 @@ RB_METHOD(FontSetDefaultName) { RB_UNUSED_PARAM; VALUE nameObj; - rb_get_args(argc, argv, "S", &nameObj); + rb_get_args(argc, argv, "S", &nameObj, RB_ARG_END); Font::setDefaultName(RSTRING_PTR(nameObj)); @@ -129,7 +129,7 @@ RB_METHOD(FontGetDefaultColor) RB_METHOD(FontSetDefaultColor) { VALUE colorObj; - rb_get_args(argc, argv, "o", &colorObj); + rb_get_args(argc, argv, "o", &colorObj, RB_ARG_END); Color *c = getPrivateDataCheck(colorObj, ColorType); diff --git a/binding-mri/graphics-binding.cpp b/binding-mri/graphics-binding.cpp index 9df69eb..7ebe441 100644 --- a/binding-mri/graphics-binding.cpp +++ b/binding-mri/graphics-binding.cpp @@ -50,7 +50,7 @@ RB_METHOD(graphicsTransition) const char *filename = 0; int vague = 40; - rb_get_args(argc, argv, "|izi", &duration, &filename, &vague); + rb_get_args(argc, argv, "|izi", &duration, &filename, &vague, RB_ARG_END); GUARD_EXC( gState->graphics().transition(duration, filename, vague); ) @@ -76,7 +76,7 @@ RB_METHOD(graphicsFrameReset) { \ RB_UNUSED_PARAM; \ int value; \ - rb_get_args(argc, argv, "i", &value); \ + rb_get_args(argc, argv, "i", &value, RB_ARG_END); \ gState->graphics().set##PropName(value); \ return rb_fix_new(value); \ } @@ -91,7 +91,7 @@ RB_METHOD(graphicsFrameReset) { \ RB_UNUSED_PARAM; \ bool value; \ - rb_get_args(argc, argv, "b", &value); \ + rb_get_args(argc, argv, "b", &value, RB_ARG_END); \ gState->graphics().set##PropName(value); \ return rb_bool_new(value); \ } diff --git a/binding-mri/input-binding.cpp b/binding-mri/input-binding.cpp index 053111e..3a07bf6 100644 --- a/binding-mri/input-binding.cpp +++ b/binding-mri/input-binding.cpp @@ -38,7 +38,7 @@ RB_METHOD(inputPress) RB_UNUSED_PARAM; int num; - rb_get_args(argc, argv, "i", &num); + rb_get_args(argc, argv, "i", &num, RB_ARG_END); Input::ButtonCode bc = (Input::ButtonCode) num; @@ -50,7 +50,7 @@ RB_METHOD(inputTrigger) RB_UNUSED_PARAM; int num; - rb_get_args(argc, argv, "i", &num); + rb_get_args(argc, argv, "i", &num, RB_ARG_END); Input::ButtonCode bc = (Input::ButtonCode) num; @@ -62,7 +62,7 @@ RB_METHOD(inputRepeat) RB_UNUSED_PARAM; int num; - rb_get_args(argc, argv, "i", &num); + rb_get_args(argc, argv, "i", &num, RB_ARG_END); Input::ButtonCode bc = (Input::ButtonCode) num; diff --git a/binding-mri/sceneelement-binding.h b/binding-mri/sceneelement-binding.h index 8f5a5f9..789764a 100644 --- a/binding-mri/sceneelement-binding.h +++ b/binding-mri/sceneelement-binding.h @@ -44,7 +44,7 @@ RB_METHOD(sceneElementSetZ) SceneElement *se = getPrivateData(self); int z; - rb_get_args(argc, argv, "i", &z); + rb_get_args(argc, argv, "i", &z, RB_ARG_END); GUARD_EXC( se->setZ(z); ); @@ -70,7 +70,7 @@ RB_METHOD(sceneElementSetVisible) SceneElement *se = getPrivateData(self); bool visible; - rb_get_args(argc, argv, "b", &visible); + rb_get_args(argc, argv, "b", &visible, RB_ARG_END); GUARD_EXC( se->setVisible(visible); ); diff --git a/binding-mri/table-binding.cpp b/binding-mri/table-binding.cpp index 224f56a..81152dd 100644 --- a/binding-mri/table-binding.cpp +++ b/binding-mri/table-binding.cpp @@ -30,7 +30,7 @@ RB_METHOD(tableInitialize) int x, y, z; x = y = z = 1; - rb_get_args(argc, argv, "i|ii", &x, &y, &z); + rb_get_args(argc, argv, "i|ii", &x, &y, &z, RB_ARG_END); Table *t = new Table(x, y, z); diff --git a/binding-mri/tilemap-binding.cpp b/binding-mri/tilemap-binding.cpp index 85f898c..8a97f3f 100644 --- a/binding-mri/tilemap-binding.cpp +++ b/binding-mri/tilemap-binding.cpp @@ -37,7 +37,7 @@ RB_METHOD(tilemapAutotilesSet) int i; VALUE bitmapObj; - rb_get_args(argc, argv, "io", &i, &bitmapObj); + rb_get_args(argc, argv, "io", &i, &bitmapObj, RB_ARG_END); Bitmap *bitmap = getPrivateDataCheck(bitmapObj, BitmapType); @@ -52,7 +52,7 @@ RB_METHOD(tilemapAutotilesSet) RB_METHOD(tilemapAutotilesGet) { int i; - rb_get_args (argc, argv, "i", &i); + rb_get_args (argc, argv, "i", &i, RB_ARG_END); if (i < 0 || i > 6) return Qnil; @@ -72,7 +72,7 @@ RB_METHOD(tilemapInitialize) VALUE viewportObj = Qnil; Viewport *viewport = 0; - rb_get_args(argc, argv, "|o", &viewportObj); + rb_get_args(argc, argv, "|o", &viewportObj, RB_ARG_END); if (rb_type(viewportObj) != RUBY_T_NIL) viewport = getPrivateDataCheck(viewportObj, ViewportType); diff --git a/binding-mri/viewport-binding.cpp b/binding-mri/viewport-binding.cpp index a450ae1..2406cc4 100644 --- a/binding-mri/viewport-binding.cpp +++ b/binding-mri/viewport-binding.cpp @@ -39,7 +39,7 @@ RB_METHOD(viewportInitialize) VALUE rectObj; Rect *rect; - rb_get_args(argc, argv, "o", &rectObj); + rb_get_args(argc, argv, "o", &rectObj, RB_ARG_END); rect = getPrivateDataCheck(rectObj, RectType); @@ -49,7 +49,7 @@ RB_METHOD(viewportInitialize) { int x, y, width, height; - rb_get_args(argc, argv, "iiii", &x, &y, &width, &height); + rb_get_args(argc, argv, "iiii", &x, &y, &width, &height, RB_ARG_END); v = new Viewport(x, y, width, height); } diff --git a/binding-mri/viewportelement-binding.h b/binding-mri/viewportelement-binding.h index e262627..996c6df 100644 --- a/binding-mri/viewportelement-binding.h +++ b/binding-mri/viewportelement-binding.h @@ -48,7 +48,7 @@ viewportElementInitialize(int argc, VALUE *argv, VALUE self) VALUE viewportObj = Qnil; Viewport *viewport = 0; - rb_get_args(argc, argv, "|o", &viewportObj); + rb_get_args(argc, argv, "|o", &viewportObj, RB_ARG_END); if (rb_type(viewportObj) != RUBY_T_NIL) viewport = getPrivateDataCheck(viewportObj, ViewportType);