Opened 6 years ago

Last modified 6 years ago

#7213 closed defect

VRTDerivedRasterBand crashes QGIS 3 on Windows (incorrect use of EnumProcessModules) — at Version 1

Reported by: Maik Riechert Owned by: warmerdam
Priority: high Milestone: 2.2.4
Component: GDAL_Raster Version: 2.2.3
Severity: blocker Keywords:
Cc:

Description (last modified by Maik Riechert)

QGIS 3 switched to Python 3 but still bundles amongst other things the python27.dll for some external tools in its bin/ folder. Only the Python 3 libraries are loaded when using QGIS 3, however.

Trying to open a VRT file with a Python-defined pixel function crashed QGIS immediately.

I tracked the problem down using WinDbg and found the offending code: https://github.com/OSGeo/gdal/blob/ef4a2ea72cba0c9f0968e84aa718f1854e41e048/gdal/frmts/vrt/vrtderivedrasterband.cpp#L464-L484

// First try in the current process in case the python symbols would
// be already loaded
HANDLE hProcess = GetCurrentProcess();
HMODULE ahModules[100];
DWORD nSizeNeeded = 0;

EnumProcessModules(hProcess, ahModules, sizeof(ahModules),
                    &nSizeNeeded);

const size_t nModules =
    std::min(size_t(100),
             static_cast<size_t>(nSizeNeeded) / sizeof(HMODULE));
for( size_t i = 0; i < nModules; i++ )
{
    if( GetProcAddress(ahModules[i], "Py_SetProgramName") )
    {
        libHandle = ahModules[i];
        CPLDebug("VRT", "Current process has python symbols loaded");
        break;
    }
}

The issue is that ~340 modules are loaded when starting QGIS and Python is one of the last ones. The GDAL code that checks whether Python is already loaded within the current process only looks at the first 100 modules, and since those don't contain Python it tries to load the DLL from the filesystem and finds the python27.dll, which of course leads to problems when loading that, since Python 3 is used in QGIS 3.

The EnumProcessModules docs say:

"It is a good idea to specify a large array of HMODULE values, because it is hard to predict how many modules there will be in the process at the time you call EnumProcessModules. To determine if the lphModule array is too small to hold all module handles for the process, compare the value returned in lpcbNeeded with the value specified in cb. If lpcbNeeded is greater than cb, increase the size of the array and call EnumProcessModules again.

To determine how many modules were enumerated by the call to EnumProcessModules, divide the resulting value in the lpcbNeeded parameter by sizeof(HMODULE)."

I suggest implementing it like they say or increase the size of ahModules to something ridiculous like 10000 which should support all reasonable cases. Since this memory is only allocated temporarily and once per lifetime of gdal I don't see a problem in doing that.

Change History (1)

comment:1 by Maik Riechert, 6 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.