From 763827c2e1fffd34e871c66d5d4c9e492c1cecec Mon Sep 17 00:00:00 2001 From: William S Fulton Date: Tue, 24 May 2016 07:33:25 +0100 Subject: [PATCH] Ruby opaque pointer handling regression fix This bug was introduced in swig-3.0.8 in #146252 adding shared_ptr support. An ObjectPreviouslyDeleted error was incorrectly thrown when the pointer was used as a parameter after being set to zero via a call to 'DATA_PTR(self) = 0'. It isn't clear to me which approach is better in this corner case, so I've gone for backwards compatibility and restored the old behaviour. Closes #602 --- CHANGES.current | 6 ++ Examples/test-suite/ruby/Makefile.in | 3 +- .../test-suite/ruby/ruby_manual_proxy_runme.rb | 49 ++++++++++++++++ Examples/test-suite/ruby_manual_proxy.i | 66 ++++++++++++++++++++++ Lib/ruby/rubyrun.swg | 12 ++-- 5 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 Examples/test-suite/ruby/ruby_manual_proxy_runme.rb create mode 100644 Examples/test-suite/ruby_manual_proxy.i #diff --git a/CHANGES.current b/CHANGES.current #index 6e347a7..f754e96 100644 #--- a/CHANGES.current #+++ b/CHANGES.current #@@ -5,6 +5,12 @@ See the RELEASENOTES file for a summary of changes in each release. # Version 3.0.9 (in progress) # =========================== # #+2016-05-23: wsfulton #+ [Ruby] Fix #602 - Error handling regression of opaque pointers introduced #+ in swig-3.0.8 when C functions explicitly reset a pointer using 'DATA_PTR(self) = 0'. #+ An ObjectPreviouslyDeleted error was incorrectly thrown when the pointer was used #+ as a parameter. #+ # 2016-05-17: tamuratak # [Ruby] Patch #651 - Correct overloaded function error message when function is # using %newobject. diff --git a/Examples/test-suite/ruby/Makefile.in b/Examples/test-suite/ruby/Makefile.in index 847c959..d94ac70 100644 --- a/Examples/test-suite/ruby/Makefile.in +++ b/Examples/test-suite/ruby/Makefile.in @@ -32,7 +32,8 @@ CPP_TEST_CASES = \ C_TEST_CASES += \ li_cdata \ - li_cstring + li_cstring \ + ruby_manual_proxy \ include $(srcdir)/../common.mk diff --git a/Examples/test-suite/ruby/ruby_manual_proxy_runme.rb b/Examples/test-suite/ruby/ruby_manual_proxy_runme.rb new file mode 100644 index 0000000..c1cee2d --- /dev/null +++ b/Examples/test-suite/ruby/ruby_manual_proxy_runme.rb @@ -0,0 +1,49 @@ +#!/usr/bin/env ruby +# +# The Subversion bindings use this manually written proxy class approach +# to the Ruby bindings. Note that in C the struct svn_fs_t is an +# opaque pointer and the Ruby FileSystem proxy class is hand written around it. +# This testcase tests this and the C close function and subsequent error +# handling. + +require 'swig_assert' +require 'ruby_manual_proxy' + +module Svn + module Fs + module_function + def create(path) + f = Ruby_manual_proxy::svn_fs_create(path) + return f + end + + FileSystem = SWIG::TYPE_p_svn_fs_t + class FileSystem + class << self + def create(*args) + Fs.create(*args) + end + end + def path + Ruby_manual_proxy::svn_fs_path(self) + end + end + end +end + +f = Svn::Fs::FileSystem.create("/tmp/myfile") +path = f.path +f.close +begin + # regression in swig-3.0.8 meant ObjectPreviouslyDeleted error was thrown instead + path = f.path + raise RuntimeError.new("IOError (1) not thrown") +rescue IOError +end + +file = nil +begin + path = Ruby_manual_proxy::svn_fs_path(file) + raise RuntimeError.new("IOError (2) not thrown") +rescue IOError +end diff --git a/Examples/test-suite/ruby_manual_proxy.i b/Examples/test-suite/ruby_manual_proxy.i new file mode 100644 index 0000000..2cb154e --- /dev/null +++ b/Examples/test-suite/ruby_manual_proxy.i @@ -0,0 +1,66 @@ +%module ruby_manual_proxy + + +%typemap(in, numinputs=0) SWIGTYPE ** ($*1_ltype temp) "$1 = &temp;"; + +%typemap(argout) SWIGTYPE **OUTPARAM { + $result = SWIG_Ruby_AppendOutput($result, SWIG_NewPointerObj(*$1, $*1_descriptor, 0)); +} + +%apply SWIGTYPE **OUTPARAM { + svn_fs_t ** +}; + +%typemap(check) svn_fs_t * { + if (!$1) { + svn_swig_rb_raise_svn_fs_already_close(); + } +} + +%{ +typedef struct svn_fs_t { + char path[256]; +} svn_fs_t; + +void svn_fs_create(svn_fs_t **fs_p, const char *path) { + svn_fs_t *fs = (svn_fs_t *)malloc(sizeof(svn_fs_t)); + strncpy(fs->path, path, 256); + *fs_p = fs; +} +const char *svn_fs_path(svn_fs_t *fs) { + return fs->path; +} +%} + +typedef struct svn_fs_t svn_fs_t; +void svn_fs_create(svn_fs_t **fs_p, const char *path); +const char *svn_fs_path(svn_fs_t *fs); + +%{ +static void svn_swig_rb_raise_svn_fs_already_close(void) { + rb_raise(rb_eIOError, "already closed"); +} + +static VALUE svn_fs_swig_rb_close(VALUE self) { + if (!DATA_PTR(self)) { + svn_swig_rb_raise_svn_fs_already_close(); + } + + DATA_PTR(self) = NULL; + + return Qnil; +} + +static VALUE svn_fs_swig_rb_closed(VALUE self) { + return DATA_PTR(self) ? Qfalse : Qtrue; +} +%} + +%insert("init") %{ + { + VALUE cSvnfs; + cSvnfs = rb_const_get(_mSWIG, rb_intern("TYPE_p_svn_fs_t")); + rb_define_method(cSvnfs, "close", + VALUEFUNC(svn_fs_swig_rb_close), 0); + } +%} diff --git a/Lib/ruby/rubyrun.swg b/Lib/ruby/rubyrun.swg index e18208f..249494a 100644 --- a/Lib/ruby/rubyrun.swg +++ b/Lib/ruby/rubyrun.swg @@ -305,6 +305,14 @@ SWIG_Ruby_ConvertPtrAndOwn(VALUE obj, void **ptr, swig_type_info *ty, int flags, /* Do type-checking if type info was provided */ if (ty) { + if (ty->clientdata) { + if (rb_obj_is_kind_of(obj, ((swig_class *) (ty->clientdata))->klass)) { + if (vptr == 0) { + /* The object has already been deleted */ + return SWIG_ObjectPreviouslyDeletedError; + } + } + } if ((c = SWIG_MangleStr(obj)) == NULL) { return SWIG_ERROR; } @@ -312,10 +320,6 @@ SWIG_Ruby_ConvertPtrAndOwn(VALUE obj, void **ptr, swig_type_info *ty, int flags, if (!tc) { return SWIG_ERROR; } else { - if (vptr == 0) { - /* The object has already been deleted */ - return SWIG_ObjectPreviouslyDeletedError; - } if (ptr) { if (tc->type == ty) { *ptr = vptr; -- 2.5.5