search

kkaempf / swig-issues

Issues from SWIG (testing)
0 stars 0 forks source link

[ruby] reinterpret_cast misused with directors #63

Open SwigAtSF opened 11 years ago

SwigAtSF commented 11 years ago

Currently, SWIG is casting classes using:

arg1 = reinterpret_cast< fltk::Widget * >(argp1); director = dynamic_cast<Swig::Director *>(arg1);

This is incorrect when directors are used, as it can lead to incorrect behavior of a multi-inheritance class. Proper casting should be: arg1 = static_cast< fltk::Widget * >(argp1); director = dynamic_cast<Swig::Director *>(arg1);

Otherwise, this results in incorrect casting behavior on x86-64 machines under Linux.

SwigAtSF commented 11 years ago

Logged In: YES user_id=961712 Originator: YES

Find attached a .zip file that shows the problem, on x86-64 machines.

Rectangle->x() returns incorrect information due to improper address/casting.

File Added: BUG.zip

SwigAtSF commented 11 years ago
SwigAtSF commented 11 years ago

Logged In: YES user_id=14972 Originator: NO

Gonzalo: Have you committed a fix for this bug?

I had a look at the code and there are some uses of reinterpret_cast<>, but they look OK. However, I don't see any uses of static_cast<> that might have replaced them...

SwigAtSF commented 11 years ago

Logged In: YES user_id=961712 Originator: YES

No, I did not. The problem does not get triggered anymore in the BUG.zip file I provided, due to the change of how swig directors now works (the order of its inheritance is now reversed), but after careful consideration I realized the bug is still subtly present. It may still show up in multiple inheritance cases or when user wraps a multiple inheritance class.

I've now checked in a fix for it. Note, however, albeit a relatively harmless fix, this effects all languages.

SwigAtSF commented 11 years ago

Logged In: YES user_id=14972 Originator: NO

I think the fix is good - it needs to affect all languages because it's currently wrong for all of them!

I just checked the testsuite output, and it's the same before and after your patch.

So marking this as fixed (if I've misunderstood, please reopen it!)

You should note this change in CHANGES.current though...

SwigAtSF commented 11 years ago

Logged In: YES user_id=961712 Originator: YES

The output is certainly different for ruby in the test suite. Only std::vector tests still use reinterpret_cast and that's okay. Also ruby uses reinterpret_cast for exceptions, but that's fine. Are you testing ruby or some other language?

Can you check also that your copy of swig has Lib/typemaps/swigtype.swg with %static_cast in them instead of %reinterpret_cast? That's all that's needed for ruby to work okay.

If you also happen to use cmake with swig, as I do, be also aware that cmake sucks in that it does not follow linux standards so that stuff located in /usr/local does not override things in /usr.

SwigAtSF commented 11 years ago

Logged In: YES user_id=14972 Originator: NO

I was testing all the languages which swig's configure script detected (since you said this affected all languages). I have the "main" ones installed, and I thought that included ruby, but for some reason I don't currently have ruby installed. (I did some work on Xapian's ruby detection recently, so I guess I must have uninstalled ruby then to test that the detection did something sensible when ruby wasn't present).

For the record, here's the list I tested with: tcl, perl5, python, java, php5, csharp.

I'm using SVN HEAD. Lib/typemaps/swigtype.swg isn't using %reinterpret_cast.

I've never used cmake.

SwigAtSF commented 11 years ago

Logged In: YES user_id=961712 Originator: YES

Well, i just gave it a try with python and there were certainly a lot of cast changes. There's probably something weird in your testing environment. With python, for example, virtual_poly_wrap will exhibit about 10-15 cast changes.

The reason I said it could effect all other languages is that Lib/typemaps/swigtype.swg is a general library function which, in theory, all other languages should be using.

I also did some some more testing with my own library stuff and I noticed that the static_cast will now prevent compiling functions that use C callbacks as parameters and have no special typemaps for them. Personally, I think that's an improvement, but it may shock users if they had incorrect code that compiled before.

SwigAtSF commented 11 years ago

Logged In: YES user_id=961712 Originator: YES

I'm looking a tad more closely to the code generated and I'm going to revert this check-in, thou.

The staticcast as I changed it is also incorrect, as swig often goes thru a void, and a staticcast of a void is also undefined and will produce results just as bad as the reinterpret_cast.

Fixing this is going to be a tad more tricky than I thought, unfortunately.

To explain what the original bug was about....

Previously, IIRC, director classes were created like:

class DirectorA : public SwigDirector, public A { };

Thus, when a redirect_cast was used like:

    A* self = redirect_cast< A* >( director );

The results were undefined. Under windows it would magically work. Under linux all would crash and burn.

With the current code generator, director classes are created like:

class DirectorA : public A, public SwigDirector { };

Thus, when a redirect_cast is used like:

    A* self = redirect_cast< A* >( director );

it works fine, as most compilers will make the base address match that of the first base class. However, I don't believe there's any guarantee in C++ that this should be so. Older C++ compilers in particular used to place the virtual table at the beginning, so the redirect_cast would crash and burn in those cases.

A C cast or a static_cast, on the other hand, guarantees that the compiler moves the pointer to the correct class location in the multiple inheritance chain, regardless of the order of definition.

Currently swig does not make any distinction in reinterpret/static_cast when directors are used, so my change effects even non-director classes. Unfortunately, swig ALSO often uses a void* in-between to obtain the address, and void* discard all type information, so a static_cast on a void* is then not guaranteed to move the pointer to the right place again (my bad fix).

Creating a test for this is now a tad tricky as most compilers do accept the reinterpret_cast as is.

The easiest way to see how this triggers something different is when a user has a function with a C callback as a parameter. Previously, swig would let that slide and then the function would often blow up at runtime. With the static_cast, the code will not compile.

Currently, the code created for directors is still wrong, but it will probably work with most popular compilers, due to the way they align data.

Ideally what you wan to have is this (sans checking code). Given a Rectangle class...

Without directors (current swig behavior):

SWIGINTERN VALUE _wrap_Rectangle_x(int argc, VALUE argv, VALUE self) { fltk::Rectangle arg1 = (fltk::Rectangle ) 0 ; int result; void argp1 = 0 ; int res1 = 0 ;

res1 = SWIG_ConvertPtr(self, &argp1,SWIGTYPE_p_fltk__Rectangle, 0 | 0 ); arg1 = reinterpret_cast< fltk::Rectangle * >(argp1); result = (int)arg1->x(); ....

}

With directors, you really want this code to be generated to be safe:

SWIGINTERN VALUE _wrap_Rectangle_x(int argc, VALUE argv, VALUE self) { fltk::Rectangle arg1 = (fltk::Rectangle ) 0 ; int result; void argp1 = 0 ; int res1 = 0 ;

res1 = SWIG_ConvertPtr(self, &argp1,SWIGTYPE_p_fltk__Rectangle, 0 | 0 ); arg1 = static_cast< fltk::Rectangle* >( reinterpret_cast< SwigDirectorRectangle * >(argp1) ); result = (int)arg1->x();

..... }

SwigAtSF commented 11 years ago

Logged In: YES user_id=14972 Originator: NO

I think you misunderstand me - when I said "I just checked the testsuite output", I mean the output on stdout and stderr from "make check-test-suite", not the code which is generated.

Your explanation makes sense to me (except I assume you mean reinterpret_cast<> where you've written redirect_cast<> a few times!)

So, reopening this bug (sigh)...