[AD] 4.2.1 showstopper bug in pack_fopen_chunk()

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

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.



AE.