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

Roger Bivand Roger.Bivand at nhh.no
Fri Feb 18 15:04:45 EST 2011


On Fri, 18 Feb 2011, Bob and Deb wrote:

> Roger,
>
> I have only 2 QGIS plugins that uses R.  They are Carson Farmer's
> manageR and Volkan Osman Kepoglu's SDA4P (see
> http://ggit.metu.edu.tr/~volkan/index.html).  Looking at the source
> code I see they both use rgdal.  I was not using these plugins when
> the crash occurred.  So, my guess is that one of the triggers to my
> problem was when one (if not both) of these plugins were registered
> (or whatever you call it) when QGIS started.
>
> After starting QGIS, I went to create a thematic map.  When I clicked
> on the Classify button, QGIS immediately crashed.  So, I turned off
> all of my plugins and enabled one plugin at a time then tried to
> create the thematic map.  When MCE Lite was enabled, QGIS crashed when
> I tried to make a thematic map.  As I said above, the 2 plugins that
> uses rgdal are manageR and SDA4P.  I'm not sure what QGIS does to
> plugins when it starts up, but I believe that was the trigger to my
> problem.
>
> I've added Volkan Osman Kepoglu to the CC list since he might be
> interested in this topic too.

Thanks, Bob, that is clearer.

Even,

I accept that "Set" is wrong and should be "Push"/"Pop".

I've committed "Push"/"Pop" to R-forge SVN - this corresponds to 1. below. 
It will be available as a source package for installation from within R on 
Saturday evening CET, I missed today's deadline.

Does CPL know which application has pushed/popped a handler - I can't see 
anything like the GEOS context handle which could be used to associate 
correctly different active programs using GDAL/OGR?

What granularity of error handler protection is needed:

1. when R loads and unloads rgdal - so that while rgdal is loaded in R, 
the R handler is at the top of the CPL error handler stack, when rgdal is 
unloaded in R, it is popped;

2. when R functions using the C/C++ interface to GDAL/OGR functions are 
entered or exit - not all the computation will be in C/C++, but will run 
continuously;

3. when C/C++ functions called by R functions, but using multiple C/C++ 
GDAL/OGR functions are entered or exit - all the computation will be in 
C/C++ but not all in GDAL/OGR, and will run continuously;

4. every time a C/C++ GDAL/OGR function is used with push just before the 
call, and pop just after the call;

Both 3. and 4. may be in loops; 4. very often are, like retrieving field 
values or geometries.

Grateful for any suggestions beyond those already given. If it has to be 
4, we'll have to take the performance hits involved.

Roger

>
> Bob
>
> On Fri, Feb 18, 2011 at 12:58 AM, Roger Bivand <Roger.Bivand at nhh.no> wrote:
>> 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
>>
>

-- 
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


More information about the gdal-dev mailing list