[gdal-dev] Potential dangerous use of CPLSetErrorHandler()

[Roger Bivand]

On Fri, 18 Feb 2011, Even Rouault wrote:

> Le vendredi 18 février 2011 01:02:30, Even Rouault a écrit :
>>
>> So my suggestion would be not to use a global error handler set when RGDAL
>> is initialized, but rather for each binding of the GDAL API, install a
>> local error handler with CPLPushErrorHandler(__errorHandler) and uninstall
>> it before returning to R with CPLPopErrorHandler()

Even,

This analysis holds in a shared object/DLL world, right? For our static 
builds of rgdal.dll for Windows 32/64-bit, there would be no interaction? 
So it's a case of dynamic linking having rather tricky pitfalls?

In GRASS, the modules linked to GDAL/OGR are short-lived, and over the 
years, nobody has reported any issues (though that isn't hard evidence), 
but QGIS and R are running together over longer periods.

I'm adding Carson Farmer to the CCs in case he is not subscribed here - he 
has some nice R/QGIS interfaces, and may be able to help in setting up a 
test rig.

Note that R runs single threaded (plus a GUI thread if there is a GUI), 
now because it is useful to run separate parallel R threads across 
multicore processors, and the R engine is small.

I feel that we need a test rig to be able to provoke the problem 
predictably, so that we know when it is settled, for known versions of 
GDAL/OGR, R, and rgdal, and versions of QGIS if we cannot provoke the 
problem without it.

From the initial report from Bob, it isn't clear (for me) how R/rgdal came 
into the picture? Is R with rgdal loaded running but separate from QGIS, 
or is R being called (even through system or via python) from QGIS?

Bob says on IRC that the problem occurs when the MCE Lite plugin is loaded 
in QGIS, but not otherwise - the plugin does not use R or rgdal. Could Bob 
confirm that this is the case, or does the problem occur independently of 
MCE Lite being loaded? Do we know which QGIS plugin is loading R and 
rgdal, and how?

Since that part of rgdal dates back at least 8 years, it may be worth 
revisiting, but I'd like to have a fuller analysis and test rig before 
modifying hundreds of functions calling GDAL and OGR in rgdal.

Roger

>>
>> For example :
>>
>> SEXP
>> RGDAL_GetDescription(SEXP sxpObj) {
>>
>>   void *pGDALObj = getGDALObjPtr(sxpObj);
>>
>>   CPLPushErrorHandler(__errorHandler);
>>
>>   const char *desc = ((GDALMajorObject *)pGDALObj)->GetDescription();
>>
>>   CPLPopErrorHandler();
>>
>>   return(mkString_safe(desc));
>>
>> }
>>
>
> Actually, I'm just thinking that when an error actually triggers, the
> CPLPopErrorHandler() will not be called due to the longjmp() done in the error
> handler... So you probably needs to call CPLPopErrorHandler() in the error
> handler before it to call error() (a quick review of port/cpl_error.cpp leads
> me to think this is OK to do so, but this needs certainly some testing)
>
> Or other possibility, it might be dangerous that the error handler interrupts
> the GDAL code flow with the longjmp(). It could cause memory leaks or leaves
> GDAL in an unstable state (unreleased locks, or whatever). What would be safer
> would be that the error handler just stores the error code and/or message, and
> just looks at it after the return from the GDAL code.
>
> Something like this :
>
> static CPLErr saved_eErrClass = CE_None;
> static saved_err_no = 0;
> static char saved_error_msg[2048];
>
> static void
> __errorHandler(CPLErr eErrClass, int err_no, const char *msg) {
> 	saved_eErrClass = eErrClass;
> 	saved_err_no = err_no;
> /* a mutex could be usefull here to avoid a race condition if 2 threads
> trigger the error handler at the same time */
> 	strncpy(saved_error_msg, msg, sizeof(saved_error_msg));
>        saved_error_msg[sizeof(saved_error_msg)-1] = 0;
> }
>
> void installErrorHandler()
> {
>   CPLPushErrorHandler(__errorHandler);
>   saved_err_no = 0;
> }
>
> void uninstallErrorHandlerAndTriggerRError()
> {
>    CPLPopErrorHandler();
>    if (saved_err_no == CE_Warning) {
>
>    warning("\n\tNon-fatal GDAL Error %d: %s\n", saved_err_no,
> saved_error_msg);
>
>  } else if (saved_err_no == CE_Failure) {
>
>    error("\n\tGDAL Error %d: %s\n", saved_err_no, saved_error_msg);
>
>  }
> }
>
> SEXP
> RGDAL_GetDescription(SEXP sxpObj) {
>
>  void *pGDALObj = getGDALObjPtr(sxpObj);
>
>  installErrorHandler();
>
>  const char *desc = ((GDALMajorObject *)pGDALObj)->GetDescription();
>
>   uninstallErrorHandlerAndTriggerRError();
>
>  return(mkString_safe(desc));
>
> }
>
> Or you could also take the approach of
> gdal/swig/include/python/python_exceptions.i where the installed error handler
> just forwards CE_Fatal, CE_Warning and CE_Debug to the previous error handler
> (or the default one if there's none). After the GDAL call, it just looks at
> CPLGetLastErrorType() and if it is CE_Failure, it triggers a Python exception
> with the error messages fetched with CPLGetLastErrorMsg().
>

-- 
Roger Bivand
Economic Geography Section, Department of Economics, Norwegian School of
Economics and Business Administration, Helleveien 30, N-5045 Bergen,
Norway. voice: +47 55 95 93 55; fax +47 55 95 95 43
e-mail: Roger.Bivand at nhh.no