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.

Análisis estático de FileOptimizer

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);
}

Análisis estático de FileOptimizer

9 comentarios en “Análisis estático de FileOptimizer”

  1. Javier Gutiérrez Chamorro (Guti)

    Muy cierto Fernando, depurar es mucho más complejo que programar. Creo que sólo optimizar es todavía más difícil que depurar.

    Conocía CRUST, aunque nunca lo he usado. Gracias por el aporte. El defecto que tiene, es que evalúa fundamentalmente la reserva y la liberación de memoria. Estos controles, ya los tienen la mayoría de compiladores de C/C++ cuando ejecutas las aplicaciones en modo debug. Incluso Visual C++ y GCC tienen detección de buffer overrrun y underrun.

    Lo que no tienen, y que era el bug que encontré es detección de leaks de recursos. En este caso, la herramienta debe saber por ejemplo que new requiere un delete, malloc un free, o CreateFileMapping requiere un CloseHandle, pero que en cambio, un MapViewOfFile necesita un UnmapViewOfFile.

  2. Javier Gutiérrez Chamorro (Guti)

    Es más complicado que eso cadenacuatro.com. El handle lo da CreateFileMapping, y luego con ese handle, el MapViewOfFile. Es decir, son dos recursos a utilizar, y que tienen que liberarse.

  3. Javier Gutiérrez Chamorro (Guti)

    Gracias Manuel. Uno se da cuenta como un sencillo proyecto, que empezó como un frontend va poco a poco complicándose.

    Si quieres, comparte con nosotros cuanto ha ahorrado en las imágenes, siempre es interesante saberlo.

  4. Uso este programa cada día para las imágenes de mi sitio. Muchas gracias.
    Un otro problema, al crear un sitio web, es el tamaño de las imágenes que debe ser el mismo para la consistencia gráfica.
    ¿Ha pensado en integrar un herramienta de conversión de tamaño?

  5. Javier Gutiérrez Chamorro (Guti)

    Muchas gracias por tu mensaje Xukyo, y sobre todo por usar FileOptimizer.

    La posibilidad de cambiar el tamaño (reescalar) imágenes, no estaba prevista inicialmente en FileOptimizer, puesto que ello implicaba modificar el archivo original, es decir, introducir pérdidas.

    Sin embargo es una función que de vez en cuando se ha solicitado (#43) y que forma parte de mi lista de tareas.

Deja un comentario