#4836 closed defect (fixed)
[patch] OCTTransform and OCTTransformEx do not check for NULL pointer
Reported by: | Leith Bade | Owned by: | warmerdam |
---|---|---|---|
Priority: | normal | Milestone: | 1.10.0 |
Component: | OGR_SRS | Version: | svn-trunk |
Severity: | normal | Keywords: | OCTTransform OCTTransformEx |
Cc: |
Description (last modified by )
I discovered a bug in
OCTTransform
and OCTTransformEx
.
The functions do not check if hTransform
is NULL
before they call hTransform->Transform()
.
As such if the caller passes in NULL
(say because creating the coordinate transformation failed on some previous step) the program crashes.
I have attached a patch against latest SVN trunk.
The code compiles but I was not able to autotest it as trunk autotest was crashing my Python.
Attachments (1)
Change History (17)
by , 12 years ago
Attachment: | ogrct.cpp.patch added |
---|
comment:1 by , 12 years ago
Summary: | OCTTransform and OCTTransformEx do not check for NULL pointer → [patch] OCTTransform and OCTTransformEx do not check for NULL pointer |
---|
comment:2 by , 12 years ago
Description: | modified (diff) |
---|
comment:3 by , 12 years ago
Milestone: | 1.9.2 → 2.0.0 |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Calling code is supposed to check the return of OGRCoordinateTransformationH(). But I've added a sanity check in trunk (r25007)
follow-up: 5 comment:4 by , 12 years ago
I had some Java code that was crashing when Proj4 could not understand the projection, thus OGRCoordinateTransformationH was NULL, and some piece of code in GDALReprojectImage did not do the supposed check.
I propably should find the problem in GDALReprojectImage too.
comment:5 by , 12 years ago
Actually, I took a closer look at it, it isn't in GDALReprojectImage it was crashing it was some Java code that was using OGRCoordinateTransformationH.
It seems that the Java bindings were not returning null when OGRCoordinateTransformationH return null.
comment:6 by , 12 years ago
Ah I see. Actually, I've found the following snippet in swig/java/apps/gdalinfo.java. The issue is that new XXXX() can never return a null pointer... The current workaround isn't really pretty. I guess that the CoordinateTransformation class should rather have a static method, and the constructor shouldn't be called :
gdal.PushErrorHandler( "CPLQuietErrorHandler" ); hTransform = new CoordinateTransformation(hProj, hLatLong); gdal.PopErrorHandler(); if (gdal.GetLastErrorMsg().indexOf("Unable to load PROJ.4 library") != -1) hTransform = null;
comment:7 by , 12 years ago
If you look at the way the C++ class is designed, it does not take the SRSs in the constructor and instead uses Initialize to take the SRSs.
The OGRCreateCoordinateTransformation function then calls Initialize.
So perhaps in v2 it might be worth changing the SWIG bindings to more closely align with the C++ API and have a static function for it instead.
I wonder if there are any other badly designed SWIG classes that have the same problem.
The SWIG code should be very robust as programmers using Java/Python/C# expect to not get crashes in the native layer as it makes it hard to debug what piece of Java/etc code caused the crash.
comment:8 by , 12 years ago
I think the same could be said about SpatialReference too.
I imagine if you pass an empty string or an invalid SRS to the constructor that it wil happily return an object then crash somewhere when you go to use it.
Most of the GDAL API classes use static methods for object creation.
I've never really done much SWIG before, but I wonder if I can create a rough patch for someone to pick up in v2 that adds static constructors for these two classes.
comment:9 by , 12 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I was working on such a patch.
SpatialReference should be OK. Even if the provided string isn't recognized, methods should behave normally
comment:10 by , 12 years ago
One more thing in OGRProj4CT::InitializeNoLock, you might want to change the if (... == NULL) with VALIDATE_POINTER1(...) for consistency.
I've gone through ogrspatialreference.cpp looking for functions missing sanity checks here are some that are missing: OGRSpatialReference::CopyGeogCSFrom - poSrcSRS OSRSetProjection - hSRS OSRSetGaussSchreiberTMercator - hSRS OGRSpatialReference::IsSameGeogCS - poOther OGRSpatialReference::IsSameVertCS - poOther OGRSpatialReference::IsSame - poOtherSRS
You can't make assumptions about what code will handle NULLs properly.
comment:11 by , 12 years ago
Bah, silly newline stripping formatter...
OGRSpatialReference::CopyGeogCSFrom - poSrcSRS
OSRSetProjection - hSRS
OSRSetGaussSchreiberTMercator - hSRS
OGRSpatialReference::IsSameGeogCS - poOther
OGRSpatialReference::IsSameVertCS - poOther
OGRSpatialReference::IsSame - poOtherSRS
comment:12 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
trunk r25015 "SWIG bindings: add a osr.CreateCoordinateTransformation(src, dst) method (and for Java, a static method CoordinateTransformation.CreateCoordinateTransformation(src, dst) (#4836)"
trunk r25016 "Add a few missing VALIDATE_POINTER1 (#4836)";
Note: I've only added them to the C functions, not the C++ method. C++ methods aren't called directly from SWIG bindings, and we would never end validating input values..., so let concentrate on the C API only. Note that the SWIG .i definition files have sometimes also null pointer checks.
comment:13 by , 12 years ago
Okay, I searched for all other potentially unsafe use of SpatialReference, CoordiateTransform and SRSNode in C functions.
These ones are more missing VALIDATE_POINTERs:
ogr_srs_ozi.cpp:
OSRImportFromOzi - hSRS
ogr_srs_panorama.cpp:
OSRImportFromPanorama - hSRS
OSRExportToPanorama - hSRS
And then these assorted ones:
ogr_srs_xml.cpp:
addProjArg - poSRS
exportAuthorityToXML - poAuthParent
exportGeogCSToXML - poSRS
exportProjCSToXML - poSRS
importXMLUnits - poSRS
importXMLAuthority - poSRS
importGeogCSFromXML - poSRS
importProjCSFromXML - poSRS
gdaltransformer.cpp - line 2136
gdaltransformer.cpp - line 972 and 973
ogr_fromepsg.cpp:
SetEPSGAxisInfo - poSRS
SetEPSGGeogCS - poSRS
SetEPSGProjCS - poSRS
SetEPSGVertCS - poSRS
SetEPSGCompdCS - poSRS
SetEPSGGeocCS - poSRS
ogr_srs_esri.cpp:
SetNewName - pOgr
RemapImgUTMNames - pOgr
RemapNameBasedOnKeyName - pOgr
RemapNamesBasedOnTwo - pOgr
RemapPValuesBasedOnProjCSAndPName - pOgr
RemapPNamesBasedOnProjCSAndPName - pOgr
DeleteParamBasedOnPrjName - pOgr
AddParamBasedOnPrjName - pOgr
RemapGeogCSName - pOgr
I did find a few more in C++ methods but you don't seem to be too worries about that.
I wonder if anyone runs a decent static analysis on GDAL? Surely it would flag a lot of this stuff.
comment:14 by , 12 years ago
I just found a small typo in the r25015 javadoc:
"For GDAL 2.0 or below, you might use the following snippet"
Should be:
"For GDAL 1.9 or below, you might use the following snippet"
comment:15 by , 12 years ago
r25019 "Add a few missing VALIDATE_POINTER1() in OSRImportFromOzi, OSRImportFromXML, OSRImportFromPanorama and OSRExportToPanorama"
I'm not convinced that the other methods you pointed need more validation. Most of them are internal methods, not in the public API, so the pointers shouldn't be NULL. And I'd note that the implicit contract for API functions is that you shouldn't pass NULL pointers, except when it is explicitely allowed. For example printf("%s", pszPtr); where pszPtr is NULL might segfault in a number of systems.
r25020 "Fix version" : thanks !
comment:16 by , 12 years ago
Well all this should fix the problem I had.
I've applied that temprary Java workaround in the meantime, so I will wait for the next release to remove it.
Thanks for making the changes for me.
Patch for ogrct.cpp