Análisis estático de FileOptimizer

Después de haberme centrado en el rendimiento de FileOptimizer, con análisis, limpiezas y optimizaciones regulares había llegado el momento de centrarse en su estabilidad y su seguridad.

Al igual que en cuanto a la eficiencia podemos guiarnos por nuestro «saber hacer», apoyarse en herramientas es algo de gran ayuda. Además de las advertencias del compilador, AQTime fue de gran ayuda.

En el caso de la detección de leaks (fugas), las herramientas de análisis estático como CppCheck pese a ser en general demasiado estrictas y poco concluyentes, admito que me ayudaron a escribir un código mejor. Siempre he prestado mucha atención a los warnings del compilador, y me ha gustado que el código escrito fuera tan bueno como fuese posible.

Motivado por las compilaciones reproducibles y la integración continua, decidí hacer uso de Coverity Scan de Synopsys, uno de los estándares de facto adoptados por la industria del software libre. No fue tarea fácil porque C++ Builder no es una herramienta que sea popular precisamente. No tan siquiera los MVP de Embarcadero pudieron ayudarme.

Finalmente lo logré, el quid de la cuestión era que los archivos .cbproj, son en realidad archivos XML de msbuild por lo que una simple linea de comandos hizo todo el trabajo:
cov-analysis-win64-2017.07\bin\cov-build –dir cov-int msbuild FileOptimizer.cbproj /t:build /p:Config=Debug

Sorprendentemente Coverity detectó un defecto, no era nada grave, pero desgraciadamente no era el que andaba buscando. Reportes de «Access Violation» que solían generarse en algunas configuraciones y que ninguno de mis equipos de desarrollo lograban reproducir.

No quedó más que la vieja técnica de ir depurando paso a paso. Tras varias sesiones me percaté que a veces fallaba la lectura de archivos. Se reportaba que no era posible leerlo puesto que había un mapeo en memoria sobre él. Siempre que es posible uso memory mapped files que ofrecen un rendimiento mucho mejor, y que además son más fáciles de manejar. Pero tras revisar el código me di cuenta que a veces el mapeo abierto con MapViewOfFile no se liberaba usando UnmapViewOfFile de manera que los siguientes accesos al archivo fallaban por ese motivo.

Como digo fue un error que no detectó ninguna herramienta, sólo el tesón y el ingenio humano fueron capaces de solucionarlo. Como siempre ocurre, una vez encontrado y corregido el problema, parece una trivialidad. Aquí tenéis la función original:

// ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------
bool __fastcall clsUtil::ReadFile(const TCHAR *pacFile, void *pvData, unsigned int *piSize, unsigned int piOffset)
{
	unsigned int iSize;
	bool bRes = false;


	HANDLE hMapping = NULL;
	HANDLE hFile = CreateFile(GetShortName((String) pacFile).c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_FLAG_SEQUENTIAL_SCAN, NULL);
	if (hFile != INVALID_HANDLE_VALUE)
	{
		if (*piSize == 0)
		{
			iSize = GetFileSize(hFile, NULL);
		}
		else
		{
			iSize = *piSize;
		}
		if (iSize > 0)
		{
			// Use file mapped IO
			hMapping = CreateFileMapping(hFile, NULL, PAGE_READONLY, 0, iSize, NULL);
			if (hMapping != INVALID_HANDLE_VALUE)
			{
				void *pacBuffer = MapViewOfFile(hMapping, FILE_MAP_READ, 0, 0, iSize);
				if ((piOffset == 0) && (pacBuffer != NULL))
				{
					memcpy(pvData, pacBuffer, iSize);
					bRes = UnmapViewOfFile(pacBuffer);
				}
				// Use regular IO
				else
				{
					if (piOffset != 0)
					{
						SetFilePointer(hFile, (long) piOffset, NULL, FILE_BEGIN);
					}
					bRes = ::ReadFile(hFile, pvData, *piSize, (unsigned long *) &iSize, NULL);
				}
			}
		}
		*piSize = iSize;
	}
	CloseHandle(hMapping);
	CloseHandle(hFile);
	return (bRes);
}

Efectivamente si MapViewOfFile retornaba NULL, o si piOffset no era cero, entonces se evitaba el acceso al archivo usando mapeos, y el fallback era la E/S estándar para ficheros. Pero quedaba bloqueado el mapeo.

Así quedó la función corregida:

// ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------
bool __fastcall clsUtil::ReadFile(const TCHAR *pacFile, void *pvData, unsigned int *piSize, unsigned int piOffset)
{
	unsigned int iSize;
	bool bRes = false;


	HANDLE hMapping = NULL;
	HANDLE hFile = CreateFile(GetShortName((String) pacFile).c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_FLAG_SEQUENTIAL_SCAN, NULL);
	if (hFile != INVALID_HANDLE_VALUE)
	{
		if (*piSize == 0)
		{
			iSize = GetFileSize(hFile, NULL);
		}
		else
		{
			iSize = *piSize;
		}
		if (iSize > 0)
		{
			// Use file mapped IO
			hMapping = CreateFileMapping(hFile, NULL, PAGE_READONLY, 0, iSize, NULL);
			if (hMapping != INVALID_HANDLE_VALUE)
			{
				void *pacBuffer = MapViewOfFile(hMapping, FILE_MAP_READ, 0, 0, iSize);
				if ((piOffset == 0) && (pacBuffer != NULL))
				{
					memcpy(pvData, pacBuffer, iSize);
					bRes = UnmapViewOfFile(pacBuffer);
				}
				// Use regular IO
				else
				{
					//Cleanup
					if (pacBuffer != NULL)
					{
						UnmapViewOfFile(pacBuffer);
					}
					if (piOffset != 0)
					{
						SetFilePointer(hFile, (long) piOffset, NULL, FILE_BEGIN);
					}
					bRes = ::ReadFile(hFile, pvData, *piSize, (unsigned long *) &iSize, NULL);
				}
			}
		}
		*piSize = iSize;
	}
	CloseHandle(hMapping);
	CloseHandle(hFile);
	return (bRes);
}