[PyQt] sipReleaseType may delete a derived type when it is 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.riverbankcomputing.com/pipermail/pyqt/attachments/20170607/94e9e45e/attachment.html>