Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#786 [ruby] reinterpret_cast misused with directors

open
Klaus Kämpf
ruby (61)
5
2012-12-10
2006-12-21
No

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.

Discussion

  •  
    Attachments
  • 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

     
  • Olly Betts
    Olly Betts
    2007-09-18

    • summary: reinterpret_cast misused with directors --> [ruby] reinterpret_cast misused with directors
    • assigned_to: cfis --> gga73
     
  • Olly Betts
    Olly Betts
    2007-09-18

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

     
  • 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.

     
  • Olly Betts
    Olly Betts
    2007-09-19

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

     
  • Olly Betts
    Olly Betts
    2007-09-19

    • status: open --> closed-fixed
     
  • 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.

     
  • Olly Betts
    Olly Betts
    2007-09-19

    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.

     
  • 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.

     
  • 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();

    .....
    }

     
  • Olly Betts
    Olly Betts
    2007-09-20

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

     
  • Olly Betts
    Olly Betts
    2007-09-20

    • status: closed-fixed --> open
     
  • William Fulton
    William Fulton
    2012-12-10

    • assigned_to: gga73 --> kkaempf