Re: [AD] 4.2.1 showstopper bug in pack_fopen_chunk()

[Peter Wang <tjaden@xxxxxxxxxx>]

On 2006-09-02, Andrei Ellman <ae-a-alleg@xxxxxxxxxx> wrote:
> 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.

Changing the test to (size >= new_size) should be enough, right?

> 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().

_AL_MALLOC/REALLOC/FREE() are the same as their libc equivalents in the
4.2 branch so I doubt it.

> 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.

Thanks.  This might be the bug you're looking for.  Can you just delete
that line and check again?

> 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.

It's not a bug since Windows allows both / and \ as path separators.

Peter