#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 )
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)
comment:1 by , 17 years ago
Description: | modified (diff) |
---|
comment:2 by , 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 , 17 years ago
Cc: | added |
---|---|
Milestone: | → 1.5.0 |
Owner: | changed from | to
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:5 by , 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 , 17 years ago
Owner: | changed from | to
---|---|
Priority: | normal → highest |
Severity: | normal → major |
Status: | new → assigned |
comment:7 by , 17 years ago
Keywords: | refactoring safety added |
---|
comment:8 by , 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.
follow-up: 20 comment:9 by , 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 , 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 , 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 , 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...
follow-up: 14 comment:13 by , 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.
follow-ups: 15 16 comment:14 by , 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
comment:15 by , 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
follow-up: 17 comment:16 by , 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
comment:17 by , 17 years ago
comment:18 by , 17 years ago
Ah, sorry confusing.. My most recent reply is related to the errors throwing fomr the GDALDeinitGCPs() function.
comment:19 by , 17 years ago
Update: pointers validation has been added to all C API calls in the ogrfeature.cpp (r11888).
comment:20 by , 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:22 by , 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 , 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 , 17 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
I've applied the improvement (r11965).
comment:25 by , 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 , 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.
Replying to ajolma: