Catch NULL strings (and others?) that need to be non-NULL with a special typemap

[Ari Jolma]

For the many string (and other?) arguments that are required to be non-NULL, we should have a swig typemap to catch them. The known ones are:

MajorObject.SetDescription 
OGR::Datasource.TestCapability
OGR::Datasource.ExecuteSQL
OGR::Layer.TestCapability
OGR::Layer.CreateField
OGR::FeatureDefn.AddFieldDefn
OGR::Geometry.* GEOS methods (at least Intersection, Disjoint, Overlaps)
OGR::Geometry.Transform
OSR.GetWellKnownGeogCSAsWKT (OSRSetWellKnownGeogCS)
OSRSetFromUserInput
OSR.GetProjectionMethodParameterList
OSR.GetProjectionMethodParamInfo
OSR::SpatialReference.GetAttrValue, SetAttrValue, GetProjParm, SetNormProjParm, GetNormProjParm, SetWellKnownGeogCS, SetFromUserInput
OSR::SpatialReference.ExportToUSGS

Additionally, can we have a generic "swig bindings" Component here?

[warmerdam]

Howard,

Do you have any ideas how to implement this? Ari?

[Mateusz Łoskot]

Here are some thoughts from the ​#gdal:

[23:46]  <hobu> re #1775 we can make a swig macro much like mloskot_dogwalk did for the c api 
[23:46]  <gdaltrac> Ticket #1775: Catch NULL strings (and others?) that need to be non-NULL with a special typemap, http://trac.osgeo.org/gdal/ticket/1775
[00:00]  * You are now known as mloskot
[00:02]  <mloskot> re 1775, in my opinion if there are requirements defined about a function arguments and these reqs have to be always fulfilled then these cases are candidates for assertions
[00:02]  <mloskot> ie. 
[00:02]  <mloskot> foo(int* p);
[00:03]  <mloskot> if p is *always* expected to be non-NULL, then 
[00:03]  <mloskot> assert(0 != p);
[00:03]  <mloskot> is strongly recommended as the first instruction of foo() function
[00:04]  <mloskot> Certainly, we can replace assert() with our own macro being active in non-DEBUG builds
[00:05]  <mloskot> but we have to be aware of potential performance penalties
[00:05]  <Nowak> mloskot: you should add "as long as it doesnt negatively affect performance"
[00:05]  <Nowak> heh, you did :D

IOW, I think input parameters should be categorized and prioritized in some way.

[Ari Jolma]

I suggest a new typemap:

%typemap(in) (char *non_null_input)

This typemap should catch fatal nulls, e.g. in the case of Perl bindings:

%typemap(in) (char *non_null_input)
{
 /* %typemap(in) (char *non_null_input) */
 if (!$1) {
   croak("argument must not be null");
   SWIG_fail;  }
} 

This typemap should be used in all those places where nulls are fatal if given to the C API, for example:

 %apply (char *non_null_input) {const char *pszNewDesc};
 void SetDescription( const char *pszNewDesc ) {
   GDALSetDescription( self, pszNewDesc );
 }
 %clear const char *pszNewDesc; 

[hobu]

Would it be less invasive to change Perl's char* typemap to not pass NULLs into GDAL functions rather than doing the %apply/%clear dance around every function in the interface?

[Ari Jolma]

Isn't this issue a problem in other languages? If it is not then of course apply/clear creates too much noise.

In some cases NULL pointers are a valid choice to GDAL but probably most cases can be cought with language-specific typemaps.

[hobu]

For example, here's some interactive output with SetDescription.

>>> ds = gdal.Open('junk.wcs')
>>> ds.SetDescription()
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "/Users/hobu/svn/gdal/swig/python/gdal.py", line 154, in SetDescription
    return _gdal.MajorObject_SetDescription(*args)
TypeError: MajorObject_SetDescription() takes exactly 2 arguments (1 given)
>>> ds.SetDescription('')
>>> ds.GetDescription()
''
>>> ds.SetDescription('some crap')
>>> ds.GetDescription()
'some crap'
>>> ds.SetDescription(None)
terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_S_construct NULL not valid
Abort trap

So, it is a problem here too.

[Ari Jolma]

I can fix all these problems with 10 specific typemaps in typemaps_perl.i, these are of type:

%typemap(check) (const char *pszNewDesc)
{
  /* %typemap(check) (const char *pszNewDesc) */
  if (!$1) {
    croak("Description must not be undefined");
    SWIG_fail;  
  }
}

Note "check" instead of "in". These do not break the usual tests I have, so perhaps they do not break other things.

[hobu]

Frank,

Ari's solution is fairly elegant, and I would like to apply this to the Python bindings as well. The catch is that we would always throw an exception. In the past these were always crashes, so I don't think it is too bad of an approach to take. Do you agree?

[Ari Jolma]

The solution is quite sensitive to argument names, which has bitten me already a couple of times. The argument name policy should be enforced somehow.

Ari

[warmerdam]

I don't really understand why this is necessary when we have null checking in the C API. Note that the C API returns an error which is turned into an exception by most swig languages *except if GDAL is built with -DDEBUG* in which case it is a fatal error (to ensure fast bug finding).

However, if you guys want extra "stuff" at the SWIG level I'm not going to interfere.

I wish it had been decided before I tasked Mateusz to put in all the C API argument checking though.

[Ari Jolma]

In fact I agree. I'll probably remove it more or less completely _once_ we have null checking in the C API.

[hobu]

I think we made great progress for 1.5 between the typemaps and C API checks. This effort isn't totally done, but we should push off these kinds of changes until 1.6 now.

[dron]

Folks,

Any ideas how to finally resolve this issue? Currently the following results in segmentation fault:

>>> from osgeo import osr
>>> srs=osr.SpatialReference()
>>> srs.ImportFromWkt(None)

[Ari Jolma]

$srs = Geo::OSR::SpatialReference->new;
$srs->ImportFromWkt(undef);

RuntimeError OGR Error: Corrupt data

The NULL from undef is converted into (probably) within the char ignorechange typemap, which is very old (not written by me). The same typemap is in every binding so maybe this particular typemap implementation is more vulnerable in Python.

I still have a bunch of typemaps catching undefined parameters (which are converted to NULLs in the bindings and may cause segfaults). Some of them might not be needed anymore in fact since there might be checks in GDAL itself - I could test them later. I don't know of any Perl code (thinking quickly right now) which segfaults so from Perl point of view this is more or less resolved.

[Ari Jolma]

meant to say (probably into "", i.e. not null)

[Even Rouault]

I think I've fixed the issue in many places by using the following generic SWIG constraint : %apply Pointer NONNULL {const char* name};