[Swig-devel] [ swig-Bugs-1620240 ] [ruby] reinterpret_cast misused with directors

[SourceForge.net <no...@so...>]

Bugs item #1620240, was opened at 2006-12-21 08:35
Message generated for change (Settings changed) made by wsfulton
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=101645&aid=1620240&group_id=1645

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: ruby
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Gonzalo Garramuno (gga73)
>Assigned to: Klaus Kämpf (kkaempf)
Summary: [ruby] reinterpret_cast misused with directors

Initial Comment:
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.


----------------------------------------------------------------------

Comment By: Olly Betts (olly)
Date: 2007-09-19 18:36

Message:
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)...

----------------------------------------------------------------------

Comment By: Gonzalo Garramuno (gga73)
Date: 2007-09-19 17:18

Message:
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 static_cast as I changed it is also incorrect, as swig often goes thru
a void*, and a static_cast 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();

.....
}


----------------------------------------------------------------------

Comment By: Gonzalo Garramuno (gga73)
Date: 2007-09-19 16:42

Message:
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.




----------------------------------------------------------------------

Comment By: Olly Betts (olly)
Date: 2007-09-19 16:04

Message:
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.

----------------------------------------------------------------------

Comment By: Gonzalo Garramuno (gga73)
Date: 2007-09-19 13:09

Message:
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.


----------------------------------------------------------------------

Comment By: Olly Betts (olly)
Date: 2007-09-19 07:19

Message:
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...

----------------------------------------------------------------------

Comment By: Gonzalo Garramuno (gga73)
Date: 2007-09-19 02:01

Message:
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.


----------------------------------------------------------------------

Comment By: Olly Betts (olly)
Date: 2007-09-17 17:58

Message:
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...

----------------------------------------------------------------------

Comment By: Gonzalo Garramuno (gga73)
Date: 2006-12-21 14:55

Message:
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

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=101645&aid=1620240&group_id=1645