Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#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 Leith Bade)

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)

ogrct.cpp.patch (668 bytes ) - added by Leith Bade 12 years ago.
Patch for ogrct.cpp

Download all attachments as: .zip

Change History (17)

by Leith Bade, 12 years ago

Attachment: ogrct.cpp.patch added

Patch for ogrct.cpp

comment:1 by Leith Bade, 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 Leith Bade, 12 years ago

Description: modified (diff)

comment:3 by Even Rouault, 12 years ago

Milestone: 1.9.22.0.0
Resolution: fixed
Status: newclosed

Calling code is supposed to check the return of OGRCoordinateTransformationH(). But I've added a sanity check in trunk (r25007)

comment:4 by Leith Bade, 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.

in reply to:  4 comment:5 by Leith Bade, 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 Even Rouault, 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 Leith Bade, 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 Leith Bade, 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 Even Rouault, 12 years ago

Resolution: fixed
Status: closedreopened

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 Leith Bade, 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 Leith Bade, 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 Even Rouault, 12 years ago

Resolution: fixed
Status: reopenedclosed

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 Leith Bade, 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 Leith Bade, 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 Even Rouault, 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 Leith Bade, 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.

Note: See TracTickets for help on using tickets.