Opened 17 years ago

Closed 17 years ago

Last modified 17 years ago

#1602 closed defect (fixed)

The validity of user supplied parameters is not checked in the API

Reported by: Ari Jolma Owned by: Mateusz Łoskot
Priority: highest Milestone: 1.5.0
Component: default Version: unspecified
Severity: major Keywords: refactoring safety
Cc: warmerdam

Description (last modified by hobu)

Many methods in the GDAL and OGR, which are also defined in the C-API, do not check if the supplied parameter is valid. Invalid parameter values, notably NULLs given where a string or a handle is expected, lead to a segfault. Since the C-API is open to many scripting languages through the SWIG, and given the way scripting languages are used, it is a common case that invalid values end up to the GDAL and OGR core. Some checks have been introduced to the SWIG interface (for example in ogr.i in OGRLayerShadow *CreateLayer?) but it is probably better to implement all these checks in the C-API. Following is a (not necessarily a comprehensive) list of classes and methods which have this defect:

 
 class Geo::GDAL
 #    GetDataTypeByName (scalar name)
 #     DecToDMS (scalar double, scalar string, scalar int)
 #     GetDriverByName (scalar ShortName)
 #     Open (scalar name, scalar Access=$Geo::GDAL::Const::GA_ReadOnly)
 #     OpenShared (scalar name, scalar Access=$Geo::GDAL::Const::GA_ReadOnly)
 #     AutoCreateWarpedVRT (scalar src_ds, scalar src_wkt=undef, scalar dst_wkt=undef, scalar ResampleAlg=$Geo::GDAL::Const::GRA_NearestNeighbour, scalar maxerror=0.0)
 class Geo::GDAL::Driver
 #     Delete (scalar name)
 class Geo::GDAL::MajorObject
 #     SetDescription (scalar NewDesc)
 class Geo::GDAL::Band
 #    ComputeBandStats (scalar samplestep=1) (if samplestep == 0, the method gets into an eternal loop?)
 class Geo::OGR
 #     GetDriverByName (scalar name)
 #     Open (scalar filename, scalar update=0)
 #     OpenShared (scalar filename, scalar update=0)
 class Geo::OGR::Driver
 #     TestCapability (scalar cap)
 class Geo::OGR::Datasource
 #    TestCapability (scalar cap)
 #    ExecuteSQL (scalar statement, scalar geom=undef, scalar dialect="")
 class Geo::OGR::Layer
 #     CreateFeature (scalar feature)
 #     TestCapability (scalar cap)
 #     CreateField (scalar field_def, scalar approx_ok=1)
 class Geo::OGR::Feature
 #     Equal (scalar feature)
 #    SetFrom (scalar other, scalar forgiving=1)
 class Geo::OGR::FeatureDefn
 #     AddFieldDefn (scalar defn)
 class Geo::OGR::Geometry
 #     Intersection (scalar other)
 #     Union (scalar other)
 #     Difference (scalar other)
 #     SymmetricDifference (scalar other)
 #     Equal (scalar other)
 #     Disjoint (scalar other)
 #     Touches (scalar other)
 #     Crosses (scalar other)
 #     Within (scalar other)
 #     Contains (scalar other)
 #     Overlaps (scalar other)
 #     Transform (scalar trans)
 class Geo::OSR
 #         GetWellKnownGeogCSAsWKT (scalar name)
 #     GetUserInputAsWKT (scalar name)
 #     GetProjectionMethodParameterList (scalar method)
 #     GetProjectionMethodParamInfo (scalar method, scalar arameter)
 class Geo::OSR::SpatialReference
 #     IsSame (scalar rhs)
 #     IsSameGeogCS (scalar rhs)
 #     GetAttrValue (scalar name, scalar child=0)
 #     SetAttrValue (scalar name, scalar value)
 #     GetProjParm (scalar name, scalar default_val=0.0)
 #     SetNormProjParm (scalar name, scalar val)
 #     GetNormProjParm (scalar name, scalar default_val=0.0)
 #     SetWellKnownGeogCS (scalar name)
 #     SetFromUserInput (scalar name)
 #     CopyGeogCSFrom (scalar rhs) 

Change History (27)

in reply to:  description comment:1 by hobu, 17 years ago

Description: modified (diff)

Replying to ajolma:

Many methods in the GDAL and OGR, which are also defined in the C-API, do not check if the supplied parameter is valid. Invalid parameter values, notably NULLs given where a string or a handle is expected, lead to a segfault. Since the C-API is open to many scripting languages through the SWIG, and given the way scripting languages are used, it is a common case that invalid values end up to the GDAL and OGR core.

Some checks have been introduced to the SWIG interface (for example in ogr.i in OGRLayerShadow *CreateLayer) but it is probably better to implement all these checks in the C-API. Following is a (not necessarily a comprehensive) list of classes and methods which have this defect: (the list should be self-explanatory, it follows the SWIG API)

class Geo::GDAL # GetDataTypeByName (scalar name) # DecToDMS (scalar double, scalar string, scalar int) # GetDriverByName (scalar ShortName) # Open (scalar name, scalar Access=$Geo::GDAL::Const::GA_ReadOnly) # OpenShared (scalar name, scalar Access=$Geo::GDAL::Const::GA_ReadOnly) # AutoCreateWarpedVRT (scalar src_ds, scalar src_wkt=undef, scalar dst_wkt=undef, scalar ResampleAlg=$Geo::GDAL::Const::GRA_NearestNeighbour, scalar maxerror=0.0) class Geo::GDAL::Driver # Delete (scalar name) class Geo::GDAL::MajorObject # SetDescription (scalar NewDesc) class Geo::GDAL::Band # ComputeBandStats (scalar samplestep=1) (if samplestep == 0, the method gets into an eternal loop?) class Geo::OGR # GetDriverByName (scalar name) # Open (scalar filename, scalar update=0) # OpenShared (scalar filename, scalar update=0) class Geo::OGR::Driver # TestCapability (scalar cap) class Geo::OGR::Datasource # TestCapability (scalar cap) # ExecuteSQL (scalar statement, scalar geom=undef, scalar dialect="") class Geo::OGR::Layer # CreateFeature (scalar feature) # TestCapability (scalar cap) # CreateField (scalar field_def, scalar approx_ok=1) class Geo::OGR::Feature # Equal (scalar feature) # SetFrom (scalar other, scalar forgiving=1) class Geo::OGR::FeatureDefn # AddFieldDefn (scalar defn) class Geo::OGR::Geometry # Intersection (scalar other) # Union (scalar other) # Difference (scalar other) # SymmetricDifference (scalar other) # Equal (scalar other) # Disjoint (scalar other) # Touches (scalar other) # Crosses (scalar other) # Within (scalar other) # Contains (scalar other) # Overlaps (scalar other) # Transform (scalar trans) class Geo::OSR # GetWellKnownGeogCSAsWKT (scalar name) # GetUserInputAsWKT (scalar name) # GetProjectionMethodParameterList (scalar method) # GetProjectionMethodParamInfo (scalar method, scalar parameter) class Geo::OSR::SpatialReference # IsSame (scalar rhs) # IsSameGeogCS (scalar rhs) # GetAttrValue (scalar name, scalar child=0) # SetAttrValue (scalar name, scalar value) # GetProjParm (scalar name, scalar default_val=0.0) # SetNormProjParm (scalar name, scalar val) # GetNormProjParm (scalar name, scalar default_val=0.0) # SetWellKnownGeogCS (scalar name) # SetFromUserInput (scalar name) # CopyGeogCSFrom (scalar rhs)

in reply to:  description comment:2 by hobu, 17 years ago

Description: modified (diff)

Replying to ajolma:

 
 class Geo::GDAL
 #    GetDataTypeByName (scalar name)
 #     DecToDMS (scalar double, scalar string, scalar int)
 #     GetDriverByName (scalar ShortName)
 #     Open (scalar name, scalar Access=$Geo::GDAL::Const::GA_ReadOnly)
 #     OpenShared (scalar name, scalar Access=$Geo::GDAL::Const::GA_ReadOnly)
 #     AutoCreateWarpedVRT (scalar src_ds, scalar src_wkt=undef, scalar dst_wkt=undef, scalar ResampleAlg=$Geo::GDAL::Const::GRA_NearestNeighbour, scalar maxerror=0.0)
 class Geo::GDAL::Driver
 #     Delete (scalar name)
 class Geo::GDAL::MajorObject
 #     SetDescription (scalar NewDesc)
 class Geo::GDAL::Band
 #    ComputeBandStats (scalar samplestep=1) (if samplestep == 0, the method gets into an eternal loop?)
 class Geo::OGR
 #     GetDriverByName (scalar name)
 #     Open (scalar filename, scalar update=0)
 #     OpenShared (scalar filename, scalar update=0)
 class Geo::OGR::Driver
 #     TestCapability (scalar cap)
 class Geo::OGR::Datasource
 #    TestCapability (scalar cap)
 #    ExecuteSQL (scalar statement, scalar geom=undef, scalar dialect="")
 class Geo::OGR::Layer
 #     CreateFeature (scalar feature)
 #     TestCapability (scalar cap)
 #     CreateField (scalar field_def, scalar approx_ok=1)
 class Geo::OGR::Feature
 #     Equal (scalar feature)
 #    SetFrom (scalar other, scalar forgiving=1)
 class Geo::OGR::FeatureDefn
 #     AddFieldDefn (scalar defn)
 class Geo::OGR::Geometry
 #     Intersection (scalar other)
 #     Union (scalar other)
 #     Difference (scalar other)
 #     SymmetricDifference (scalar other)
 #     Equal (scalar other)
 #     Disjoint (scalar other)
 #     Touches (scalar other)
 #     Crosses (scalar other)
 #     Within (scalar other)
 #     Contains (scalar other)
 #     Overlaps (scalar other)
 #     Transform (scalar trans)
 class Geo::OSR
 #         GetWellKnownGeogCSAsWKT (scalar name)
 #     GetUserInputAsWKT (scalar name)
 #     GetProjectionMethodParameterList (scalar method)
 #     GetProjectionMethodParamInfo (scalar method, scalar arameter)
 class Geo::OSR::SpatialReference
 #     IsSame (scalar rhs)
 #     IsSameGeogCS (scalar rhs)
 #     GetAttrValue (scalar name, scalar child=0)
 #     SetAttrValue (scalar name, scalar value)
 #     GetProjParm (scalar name, scalar default_val=0.0)
 #     SetNormProjParm (scalar name, scalar val)
 #     GetNormProjParm (scalar name, scalar default_val=0.0)
 #     SetWellKnownGeogCS (scalar name)
 #     SetFromUserInput (scalar name)
 #     CopyGeogCSFrom (scalar rhs) 

comment:3 by warmerdam, 17 years ago

Cc: warmerdam added
Milestone: 1.5.0
Owner: changed from warmerdam to mlsskot

I would like to see NULL checks in the whole GDAL/OGR C API.

Turning over to Mateusz, aimed at 1.5.0. The objective is to avoid crashes, to issue an error if the object is NULL, and to let processing continue as gracefully as is reasonable.

So:

void CPL_STDCALL GDALFlushCache( GDALDatasetH hDS )

{
    ((GDALDataset *) hDS)->FlushCache();
}

Might become:

void CPL_STDCALL GDALFlushCache( GDALDatasetH hDS )

{
     if( hDS == NULL )
     {
         CPLError( CE_Failure, CPLE_ObjectNull, "Dataset NULL in GDALFlushCache()." );
         return;
     }
 
     ((GDALDataset *) hDS)->FlushCache();
}

CPLE_ObjectNull will need to be added to cpl_error.h.

comment:4 by hobu, 17 years ago

Geometry.Equal done in r11753

comment:5 by Ari Jolma, 17 years ago

A test for layer name added to OGR_DS_CreateLayer in ogrdatasource.cpp and the same test removed from ogr.i in 11760. A test for this is added into swig Perl tests.

I could do more of these similarly if that's ok.

Ari

comment:6 by Mateusz Łoskot, 17 years ago

Owner: changed from mlsskot to Mateusz Łoskot
Priority: normalhighest
Severity: normalmajor
Status: newassigned

comment:7 by Mateusz Łoskot, 17 years ago

Keywords: refactoring safety added

comment:8 by Mateusz Łoskot, 17 years ago

I've taken over this ticket.

Let's treat it more as a longer term ticket than a bug fixed in one shot. I'm going to review every class and add appropriate tests in (roughly) following order: 1) Ari's list 2) general classes 3) then move on with reviewing classes of drivers

Obviously, it will take some time.

comment:9 by Mateusz Łoskot, 17 years ago

Folks,

Do you think it's reasonable to change behavior of the CPLError() a little for the new error type - CPLE_ObjectNull? How we should handle invalid parameters, aborting or continuing execution? If error of CE_Fatal class occures, GDAL aborts execution. Should we consider CPLE_ObjectNull as CE_Failure (no abort) or may be as CE_Fatal?

comment:10 by hobu, 17 years ago

I don't think CPLE_ObjectNull should be a CE_Fatal. It is not uncommon for the binding languages to inadvertently pass in a NULL. We shouldn't be tearing down their interpreter because of it. If the error is set, in an exception will be thrown in languages that set it. In the C API, the user should be checking the return types of the functions and looking for an error. Both of these scenarios mean to me that doing CE_Fatal is not required...

comment:11 by Mateusz Łoskot, 17 years ago

I'm not sure about following functions:

GDALGetDriverShortName
GDALGetDriverShortName
GDALGetDriverHelpTopic
GDALGetDriverCreationOptionList

All these functions accept handle to a driver and test it this way:

    if( hDriver == NULL )
        return NULL;

It looks like a designed logic and I'm wondering if it's a good idea to replace it with validation test throwing CPL error, for example:

     if( hDriver == NULL )
     {
         CPLError( CE_Failure, CPLE_ObjectNull, "Driver is NULL." );
         return NULL;
     }

Any idea?

comment:12 by hobu, 17 years ago

Yes, set the CPLError. I think in the past we just failed silently, but now we are trying to be more complete with the errors...

comment:13 by Mateusz Łoskot, 17 years ago

Recently, I've been working on this ticket. Current state of input parameters (mostly pointers) validation is that there are now 270 checks in GDAL/OGR C API calls. It roughly translates to around 220-240 API calls covered with validation.

Please, if anyone could take a look at the validation mechanism and review if it works as expected for scripting languages, I'd be thankful.

The work is in progress.

in reply to:  13 ; comment:14 by Ari Jolma, 17 years ago

Thanks, that seems to fix many of the core dumps I get in my test code, the remaining methods causing problems with null arguments seem to be (after a not very thorough research):

MajorObject.SetDescription
MajorObject.ComputeBandStats (samplestep = 0 leads to a infinite loop I think)
TestCapability in OGR driver and other classes
Datasource.ExecuteSQL
Layer.CreateFeature
Layer.CreateField
Feature.Equal
Feature.SetFrom
FeatureDefn.AddFieldDefn
Geometry most GEOS methods (Intersection, Union etc)
OSR.GetWellKnownGeogCSAsWKT (and others)
SpatialReference.GetAttrValue (and Set..)
SpatialReference.*Parm
SpatialReference.SetWellKnownGeogCS
SpatialReference.SetFromUserInput
SpatialReference.CopyGeogCSFrom
SpatialReference.ExportToUSGS

there is also some problem now with Feature.new that was not before and also in Perl test code there is now a complaint about psGCP being null in GDALDeinitGCPs that was not there before

in reply to:  14 comment:15 by Ari Jolma, 17 years ago

there is now a complaint about psGCP being null in GDALDeinitGCPs that was not there before

this is because in memdataset.cpp MEMDataset delete calls blindly GDALDeinitGCPs

in reply to:  14 ; comment:16 by Ari Jolma, 17 years ago

there is also some problem now with Feature.new that was not before

this problem was also before, it is due to OGR_F_CREATE not testing the validity of hDefn

in reply to:  16 comment:17 by Mateusz Łoskot, 17 years ago

Replying to ajolma:

there is also some problem now with Feature.new that was not before

this problem was also before, it is due to OGR_F_CREATE not testing the validity of hDefn

Ari, thanks for investigating this.

I applied tiny fix (r11887) and it's not flooding with any error from this function.

comment:18 by Mateusz Łoskot, 17 years ago

Ah, sorry confusing.. My most recent reply is related to the errors throwing fomr the GDALDeinitGCPs() function.

comment:19 by Mateusz Łoskot, 17 years ago

Update: pointers validation has been added to all C API calls in the ogrfeature.cpp (r11888).

in reply to:  9 comment:20 by warmerdam, 17 years ago

Replying to mloskot:

Folks,

Do you think it's reasonable to change behavior of the CPLError() a little for the new error type - CPLE_ObjectNull? How we should handle invalid parameters, aborting or continuing execution? If error of CE_Fatal class occures, GDAL aborts execution. Should we consider CPLE_ObjectNull as CE_Failure (no abort) or may be as CE_Fatal?

I concur with hobu's position on this. object as null should be treated as CE_Failure, not fatal.

comment:21 by Mateusz Łoskot, 17 years ago

Frank,

Understood. I report CE_Failure in validation I added.

comment:22 by Mateusz Łoskot, 17 years ago

I added new error code to OGRErr values: OGRERR_INVALID_HANDLE (r11918). This code is returned from OGR if validation of passed handler fails. (Work on validation is in progress)

comment:23 by warmerdam, 17 years ago

Mateusz,

As per our discussion with Andrey, could you pass CE_Fatal if the DEBUG macro is defined, otherwise pass CE_Failure? That way development builds should crash and provide handy traceback info while production builds will hopefully exhibit some degree of resilience.

comment:24 by Mateusz Łoskot, 17 years ago

Resolution: fixed
Status: assignedclosed

I've applied the improvement (r11965).

comment:25 by Ari Jolma, 17 years ago

Sorry to spoil the fun but most of the segfaults in my (short) list above (08/15/07) are still there. For example ogrsfdriver.cpp:150 should also(?) test pszCap and ogrdatasource.cpp:847 similarly etc. Feature.Equal is ok.

Should this be reopened?

comment:26 by warmerdam, 17 years ago

It is not my opinion that we need to check all parameters. Object pointers were of particular interest because of how easy it is for them to come back NULL from other functions.

If we want to do broader argument checking I think it should first be discussed on the gdal-dev list, and handled via a seperate task.

comment:27 by Mateusz Łoskot, 17 years ago

I agree with Frank.

Note: See TracTickets for help on using tickets.