[PyQt] sipReleaseType may destroy a derived type when it's not

[Denis Rivière]

Hi,

We experience crashes using sip 4.18 and 4.19, where it works fine using 
4.17.
I have dug for a while in the code, and found out that in some cases, 
after object conversion into a class using its %ConvertToTypeCode, the 
object gets deleted after a cast to the wrong sip-derived class.

Let's say I have a C++ class AClass, with SIP bindings and conversion 
code from another type (int for instance) which instantiates a new AClass.

In another custom function (named print_float_or_aclass() in the 
example, which is quite pointless but it's just an example), the 
conversion code is invoked, the converted object is used, then 
sipReleaseType() is normally called. But this release destroys the 
object after cast to sipAClass, where it is really a AClass.

I tried to make a relatively small example to demonstrate the problem.

file: aclass.h

#ifndef ACLASS_H
#define ACLASS_H

class AClass
{
public:
   AClass( int x );
   AClass( const AClass & x );
   virtual ~AClass();

   int number() const;

private:
   int _number;
};

#endif

--

file: aclass.cpp

#include "aclass.h"


AClass::AClass( int x )
   : _number( x )
{
}

AClass::AClass( const AClass & x )
   : _number( x.number() )
{
}

AClass::~AClass()
{
}


int AClass::number() const
{
   return _number;
}

--

file: aclass.sip


%Module aclass

class AClass
{
public:
%TypeHeaderCode
#include "aclass.h"
%End

%ConvertToTypeCode

   if( sipIsErr == NULL )
   {
     if( PyInt_Check( sipPy ) )
       return 1;
     else
       return 0;
   }
   else
   {
     std::cout << "ConvertToTypeCode, instantiate new AClass\n";
     AClass *aclass = new AClass( PyInt_AsLong( sipPy ) );
     *sipCppPtr = aclass;
     return sipGetState( sipTransferObj );
   }

%End

   AClass( int x );
   AClass( const AClass & x );
   virtual ~AClass();
   int number() const;
};

%ModuleCode
#include "aclass.h"
#include <iostream>
%End

void print_float_or_aclass( SIP_PYOBJECT );
%MethodCode
   if( PyFloat_Check( a0 ) )
     std::cout << "float: " << PyFloat_AsDouble( a0 ) << std::endl;
   else
   {
     int state = 0;
     void *obj = sipForceConvertToType( a0, sipType_AClass,
           0, 0, &state, &sipIsErr );
     if( obj )
     {
       AClass *o = reinterpret_cast<AClass *>( obj );
       std::cout << "AClass: " << o->number() << std::endl;
       sipReleaseType( obj, sipType_AClass, state );
     }
   }
%End

--

this (very) minimal Makefile can be used to build it from the source 
directory:

PYTHON_INCLUDE=/usr/include/python2.7

# comment out the following 2 lines and uncomment the next to use sip 4.19
SIP_INCLUDE=/usr/include/python2.7
SIP_EXE=sip

# SIP_INCLUDE=/home/someone/sip-4.19.2/include
# SIP_EXE=/home/someone/sip-4.19.2/bin/sip

all:    aclass.so

sipaclasspart0.cpp: aclass.sip aclass.cpp aclass.h
     $(SIP_EXE) -j1 -c . aclass.sip

aclass.so:    sipaclasspart0.cpp
     g++ -fPIC -shared -o aclass.so -I$(SIP_INCLUDE) -I$(PYTHON_INCLUDE) 
sipaclasspart0.cpp aclass.cpp

--

When using for instance aclass.print_float_or_aclass(12), 12 (int) gets 
converted to a AClass object, which is released after use.
But the release code does not do the same when using sip 4.17 or 
4.18/4.19. We can print things by slighty modifying the code of the 
release_AClass() function in the generated file sipclasspart0.cpp:

with sip 4.19.2:

/* Call the instance's destructor. */
extern "C" {static void release_AClass(void *, int);}
static void release_AClass(void *sipCppV, int sipIsDerived)
{
     std::cout << "release_AClass, sipIsDerived: " << sipIsDerived << 
std::endl;
     if (sipIsDerived)
     {
         std::cout << "delete derived\n";
         delete reinterpret_cast<sipAClass *>(sipCppV);
     }
     else
     {
         std::cout << "delete AClass\n";
         delete reinterpret_cast< ::AClass *>(sipCppV);
     }
}


with sip 4.17:

/* Call the instance's destructor. */
extern "C" {static void release_AClass(void *, int);}
static void release_AClass(void *sipCppV,int sipState)
{
     std::cout << "release_AClass, sipState: " << sipState << std::endl;
     if (sipState & SIP_DERIVED_CLASS)
     {
         std::cout << "delete derived\n";
         delete reinterpret_cast<sipAClass *>(sipCppV);
     }
     else
     {
         std::cout << "delete AClass\n";
         delete reinterpret_cast<AClass *>(sipCppV);
     }
}

Then after rebuilding the module, we use the function:

 >>> import aclass
 >>> aclass.print_float_or_aclass(12)

with sip 4.17, it prints:

ConvertToTypeCode, instantiate new AClass
AClass: 12
release_AClass, sipState: 1
delete AClass

(so it is not the derived class case)

with sip 4.19 (I have changed PYTHONPATH to point to the sip 4.19 module 
before running), it prints:

ConvertToTypeCode, instantiate new AClass
AClass: 12
release_AClass, sipIsDerived: 1
delete derived

(note that it does not crash every time, but seems wrong anyway)

Here the release code is called with sipIsDerived argument set to 1. 
However 1 is actually directly the "state" value set in 
%ConvertToTypeCode, which does not mean SIP_DERIVED_CLASS (which value 
is 2), but SIP_TEMPORARY.

In the source code of siplib (siplib.c.in), the code of 
sip_api_release_type() calls the release function of the type object 
with complete "state" value, not a boolean meaning if it is derived.

So I think there is a bug there.
Am I right ?

Regards,
Denis