Re: [AD] [4.4]/[4.2] [Windows] Problems with gfx_gdi_init()/gfx_gdi_exit()

[Andrei Ellman <ae-a-alleg@xxxxxxxxxx>]

On 22/02/2011 01:06, Peter Wang wrote:
> On 2011-02-21, Andrei Ellman<ae-a-alleg@xxxxxxxxxx>  wrote:
> > Hi,
> >
> > In Windows (using Allegro 4.2.3.1) if I successfully set a graphics mode
> > using the GFX_GDI driver and then set a different mode using GFX_GDI
> > that causes gfx_gdi_init() to fail, then a crash occurs in gfx_gdi_exit().
> >
> > When examining wgdi.c, in gfx_gdi_exit() I noticed that 'screen_surf' is
> > not set to NULL once it has been free()'d, which was the cause of the
> > crash (as it was already free()'d on the previous successful removal of
> > that mode). Also, I notice that _AL_FREE(gdi_dirty_lines); and
> > _AL_FREE(screen_surf); don't check to see if the pointer is NULL.
> > Although 'gdi_screen' is set to NULL, none of it's associated
> > datastructures (as allocated in _make_bitmap()) are free()'d, causing a
> > memory-leak. Also, in gfx_gdi_init(), there are no checks done to see if
> > _AL_MALLOC_ATOMIC() or _make_bitmap() return NULL.
> >
> > The code of wgdi.c is identical in 4.2.3.1 and 4.4.1.1 so any fix could
> > be applied to both branches.
> A patch would be appreciated.
>
> Peter


At last, I've found some time to look into this and have produced a 
patch. The patch addresses all the issues I mentioned (wgdi.c.diff) (and 
one I didn't mention (wddovl.c.diff)) except the one about the 
memory-leak which turned out to be a non-issue as the bitmap is free()'d 
by the caller.

Here is a summary of the changes.

wddovl.c
+ Fails instead of crashes if it can't create a bitmap from the surface 
(if gfx_directx_make_bitmap_from_surface() fails).
wgdi.c
+ gfx_gdi_exit(): No longer crashes if attempting to set a GFX_GDI 
graphics mode fails after a previous attempt to setup a GFX_GDI graphics 
mode (GDIExitCrash.c should crash without the patch and not crash with 
the patch). This is done by setting the static global screen_surf to 
NULL when it has been free'd.
+ gfx_gdi_exit(): Checks gdi_dirty_lines and screen_surf pointers for 
NULL before it free's them.
+ gfx_gdi_exit(): vsync_event is set to NULL once the handle has been 
closed.
+ gfx_gdi_init(): Instead of ASSERT'ing _AL_MALLOC_ATOMIC() for 
gdi_dirty_lines succeeds, gfx_gdi_init() now fails.
+ gfx_gdi_init(): if _make_bitmap() fails, gfx_gdi_init() now fails.

Note that wddovl.c and wgdi.c are identical in both 4.2.3.1 and 4.4.1.1 
so the patch can be applied to both branches.

I've included a test-program GDIExitCrash.c that crashes without the 
patch and behaves as expected with the patch (I've only tested this in 
DEBUG-mode in MSVC).


AE.

--- wddovl.old.c	2011-03-14 14:27:06.482152000 +0100
+++ wddovl.c	2011-03-14 13:54:55.405400000 +0100
@@ -378,6 +378,10 @@
       goto Error;
 
    gfx_directx_forefront_bitmap = gfx_directx_make_bitmap_from_surface(overlay_surface, w, h, BMP_ID_VIDEO);
+   if (!gfx_directx_forefront_bitmap) {
+      _TRACE(PREFIX_E "Can't make a bitmap from the surface\n");
+      goto Error;
+   }
 
    /* set the overlay color key */
    key.dwColorSpaceLowValue = gfx_directx_forefront_bitmap->vtable->mask_color;

--- wgdi.old.c	2011-03-14 14:27:16.426451200 +0100
+++ wgdi.c	2011-03-15 02:29:41.713849600 +0100
@@ -155,7 +155,7 @@
 static int lock_nesting = 0;
 static int render_semaphore = FALSE;
 static PALETTE palette;
-static HANDLE vsync_event;
+static HANDLE vsync_event = NULL;
 #define RENDER_DELAY (1000/70)  /* 70 Hz */
 
 /* hardware mouse cursor emulation */
@@ -515,13 +515,20 @@
 
    /* the last flag serves as an end of loop delimiter */
    gdi_dirty_lines = _AL_MALLOC_ATOMIC((h+1) * sizeof(char));
-   ASSERT(gdi_dirty_lines);
+   if (!gdi_dirty_lines) {
+      _TRACE(PREFIX_E "Could not allocate memory for gdi_dirty_lines.\n");	   
+      goto Error;
+   }
    memset(gdi_dirty_lines, 0, (h+1) * sizeof(char));
    gdi_dirty_lines[h] = 1;
 
    /* create the screen surface */
    screen_surf = _AL_MALLOC_ATOMIC(w * h * BYTES_PER_PIXEL(color_depth));
    gdi_screen = _make_bitmap(w, h, (unsigned long)screen_surf, &gfx_gdi, color_depth, w * BYTES_PER_PIXEL(color_depth));
+   if (!gdi_screen) {
+      _TRACE(PREFIX_E "Could not make a bitmap out of the screen surface.\n");	   
+      goto Error;
+   }
    gdi_screen->write_bank = gfx_gdi_write_bank; 
    _screen_vtable.acquire = gfx_gdi_lock;
    _screen_vtable.release = gfx_gdi_unlock;
@@ -570,16 +577,23 @@
    /* stop timer */
    remove_int(render_proc);
    CloseHandle(vsync_event);
+   vsync_event = NULL;
 
    /* disconnect from the system driver */
    win_gfx_driver = NULL;
 
-   /* destroy dirty lines array */   
-   _AL_FREE(gdi_dirty_lines);
-   gdi_dirty_lines = NULL;   
+   /* destroy dirty lines array */
+   if (gdi_dirty_lines) {
+      _AL_FREE(gdi_dirty_lines);
+      gdi_dirty_lines = NULL;
+   }
 
    /* destroy screen surface */
-   _AL_FREE(screen_surf);
+   if (screen_surf) {
+      _AL_FREE(screen_surf);
+      screen_surf = NULL;
+   }
+
    gdi_screen = NULL;
 
    /* destroy mouse bitmaps */

/***************************************************************************
 *
 * GDIExitCrash.c
 * Test program to show the crash in gfx_gdi_exit().
 * Must be built for Windows.
 *
 **************************************************************************/


#include <allegro.h>



int main(void)
{
	if(allegro_init())
	{
		return -1;
	}




	/* Firstly, set a sane mode */
	set_gfx_mode(GFX_GDI, 640, 480, 0, 0);


	/* Now set an impossibly large mode that's bound to fail */
	set_gfx_mode(GFX_GDI, 30000, 30000, 0, 0);
	/* Without the patch, the above line will cause a crash. */




	allegro_exit();


	return 0;
}
END_OF_MAIN();