Opened 9 years ago
Closed 9 years ago
#6113 closed defect (fixed)
Dead Code in ogr_fromepsg.cpp
Reported by: | Kyle Shannon | Owned by: | Kurt Schwehr |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | OGR_SRS | Version: | svn-trunk |
Severity: | normal | Keywords: | |
Cc: | Kurt Schwehr |
Description
The return value at:
https://trac.osgeo.org/gdal/browser/trunk/gdal/ogr/ogr_fromepsg.cpp#L228
is never NULL. CVSGetField() returns an empty string, never NULL:
https://trac.osgeo.org/gdal/browser/trunk/gdal/port/cpl_csv.cpp#L980
so the else clause of
if( pszUOMName != NULL ){...} else {...}
has never been executed, and never tested:
(line 266).
Possible fix:
Index: ogr/ogr_fromepsg.cpp =================================================================== --- ogr/ogr_fromepsg.cpp (revision 30295) +++ ogr/ogr_fromepsg.cpp (working copy) @@ -228,7 +228,7 @@ const char *pszUOMName = CSVGetField( pszFilename, "UOM_CODE", szSearchKey, CC_Integer, "UNIT_OF_MEAS_NAME" ); - + CPLAssert( pszUOMName ); /* -------------------------------------------------------------------- */ /* If the file is found, read from there. Note that FactorC is */ /* an empty field for any of the DMS style formats, and in this */ @@ -237,7 +237,7 @@ /* -------------------------------------------------------------------- */ double dfInDegrees = 1.0; - if( pszUOMName != NULL ) + if( !EQUAL( pszUOMName, "" ) ) { const double dfFactorB = CPLAtof(CSVGetField( pszFilename, @@ -304,14 +304,7 @@ /* Return to caller. */ /* -------------------------------------------------------------------- */ if( ppszUOMName != NULL ) - { - // CPLStrdup returns a copy of "" if given a nullptr. - if( pszUOMName != NULL ) - *ppszUOMName = CPLStrdup( pszUOMName ); - else - *ppszUOMName = NULL; - } - + *ppszUOMName = CPLStrdup( pszUOMName ); if( pdfInDegrees != NULL ) *pdfInDegrees = dfInDegrees;
goatbar has been working in this area while fixing coverity, probably best for him to look it over(CID:1074894).
It was difficult to create a test test, as the csv file would have to inaccessible for EPSGGetUOMAngleInfo(), but accessible by the caller OGRSpatialReference::importFromEPSGA();
Change History (2)
comment:1 by , 9 years ago
Owner: | changed from | to
---|
comment:2 by , 9 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
trunk r30755 "CSVGetField(): make it (more) obvious that the function cannot return nullptr (#6113, CID 1074894)"
trunk r30756 "Remove useless comparison to nullptr since CSVGetField() cannot return nullptr (patch by KyleS, #6113, CID 1074894)"
If Coverity still complains, it is just too stupid ;-) It should already have understood the semantics of CSLCount()
I'm looking at it. I still haven't proven that CVSGetField() never returns a nullptr.
Work includes r30388 r30389 r30392 trying to see the flow a little easier. This has to always be not a nullptr:
I'm pretty sure that is never a nullptr, but it's hard to prove.