[AD] 4.2.1 showstopper bug in pack_fopen_chunk()

[ Thread Index | Date Index | More lists.liballeg.org/allegro-developers Archives ]

I have discovered two bugs in pack_fopen_chunk() in file.c . One (and possibly both) are to do with the Windows-related code. I found this when debugging a DEBUG build of my app using MSVC6 and the Allegro DEBUG DLL build in Windows 2000.

Firstly, in the following code in in pack_fopen_chunk() starting on line 1960 of file.c of 4.2.1 (this piece of code was introduced after 4.2.0),

        /* Get the path of the temporary directory */
        do {
           size = new_size;
           tmp_dir = realloc(tmp_dir, size);
           new_size = GetTempPath(size, tmp_dir);
        } while ( (size > new_size) && (new_size > 0) );

there is an overflow. According to the docs for GetTempPath() at http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/fs/gettemppath.asp , the size returned by the function does not take the terminating NULL into account. This means that at the next call to realloc(), the string is one byte short, and on the next call to GetTempPath(), it somehow ends up without a terminating \0 character.

The second bug involves memory being freed somewhere where it shouldn't be. For some reason, if I call pack_fopen_chunk() with f->normal.flags & PACKFILE_FLAG_WRITE for the first time, things seem to be OK, but if I call it again with f->normal.flags & PACKFILE_FLAG_WRITE, then some memory that I malloced just before the call to pack_fopen_chunk() that was OK on the first call but seems to be messed around with on the second call (this might have had something to do with memory being freed (this I know because the memory in the datastructure that I malloced earlier contains "FEEEFEEE" which is MSVC's way of saying that the memory in question has been free()'d by free(), whereas things worked just fine with 4.2.0. BTW. my program uses the bog-standard malloc() and free())).

I have a suspicion that the bug above might involve mixing stdlib's 'realloc() (as used in the code-fragment I posted earlier)' with Allegro's _AL_FREE(). Also, I notice that _AL_FREE() is used to free the string generated by the following line (2007)

        tmpnam_string = tmpnam(NULL);

However, according to http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/HTML/_crt__tempnam.2c_._wtempnam.2c_.tmpnam.2c_._wtmpnam.asp tmpnam() returns a pointer to an internal buffer, rather than an allocated string, so it does not need to be freed.

Although the above should give some idea of what is going wrong, I'm not 100% sure, so have not provided a fix.

Incidentally, I notice that on line 2009, there is the line
sprintf(tmp_name, "%s/%s", tmp_dir, tmpnam_string);
As well as creating tmp_name with forward-slash on Windows, tmp_dir has a trailing bakslash, and , tmpnam_string has a leading backslash, and the resulting path is soomething like C:\DOCUME~1\ADMINI~1\LOCALS~1\Temp\/\s17o. Fortunately, The '\/\' sequence does not cause any harm, and the file 's17o.' is created in the proper directory.

As I'm going on Holiday shortly, I won't have any time to test the fixes for at least two weeks, but would appreciate it if someone could test a Windows Debug build of a program using the Allegro Debug DLL that calls pack_fopen_chunk() at least twice.


Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/