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

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

OK, I've returned from my holiday, and have come up with a patch that 
fixes both the GetTempPath() string-size bug (Windows) and the 
free()'ing tmpnam_string bug (all platforms).

The fix for the GetTempPath() bug actually involved changing the '>' to 
a '<'. I had originally assumed that the code was trying to save memory 
by pruning the excess length from the string-buffer (which I assumed 
when I single-stepped the code in the debugger), but then realised that 
it was supposed to grow the buffer if it was too small (which it failed 
to do, but because the default length was sufficiently large, this did 
not show up). While GetTempPath() does not add the terminating \0 to the 
size it returns if the buffer was sufficiently large, it does include it 
when the buffer is too small. Also, GetTempPath()'s nBufferLength 
paramater and return value are actually lengths in TCHARs, so I also 
modified the realloc() call to realloc(tmp_dir, size * sizeof(TCHAR)).

As for the free()'ing tmpnam_string bug, as well as looking it up on the 
microsoft site as I did in my last post, I also looked it up in the libc 
reference at DJ Delorie's site, and came to the conclusion that the 
return value of tmpnam(NULL) always points to an internal buffer. This 
meant all I had to do was get rid of the line that free()'d tmpnam_string.

While this fixes the bugs, there are still some style-related issues in 
pack_fopen_chunk() that I have not fixed. There apperas to be some 
inconsistencies in the usage of _AL_MALLOC/_AL_FREE etc. and malloc/free 
. For example, the lines
         _AL_FREE(tmp_dir);
         _AL_FREE(tmp_name);
appear, and a few lines later,
      free(tmp_dir);
      free(tmp_name);
appear.
At the moment, _AL_MALLOC/_AL_FREE are the same as their libc 
equivalents in the 4.2 branch, but if this were to change, things could 
start to go wrong.

Also, I've not fixed the issue of the temporary filename under Windows 
containing a '\/\' sequence it it's path, although W2K manages to cope 
with it and treat it like a regular '\'.

Anyway, I've tried this fix out with the code that caused the original 
problem to show up, and the resulting patch has eliminated the problem. 
Note that the patch is against the version of 4.2.1 that Evert posted 
some 3 weeks ago (this might be RC2). At the moment, I've only tested it 
on W2K, MSVC6, Debug-DLL and Debug-Staticlink builds.


AE.

*** file.c.old	Sun Jul 16 22:16:48 2006
--- file.c	Tue Sep 19 00:45:43 2006
*************** PACKFILE *pack_fopen_chunk(PACKFILE *f, 
*** 1960,1968 ****
           /* 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) );
           
           /* Check if we retrieved the path OK */
           if (new_size == 0)
--- 1960,1968 ----
           /* Get the path of the temporary directory */
           do {
              size = new_size;
!             tmp_dir = realloc(tmp_dir, size * sizeof(TCHAR));
              new_size = GetTempPath(size, tmp_dir);
!          } while ( (size < new_size) && (new_size > 0) );
           
           /* Check if we retrieved the path OK */
           if (new_size == 0)
*************** PACKFILE *pack_fopen_chunk(PACKFILE *f, 
*** 2004,2013 ****
           /* note: since the filename creation and the opening are not
            * an atomic operation, this is not secure
            */
!          tmpnam_string = tmpnam(NULL);
           tmp_name = _AL_MALLOC_ATOMIC(strlen(tmp_dir) + strlen(tmpnam_string) + 2);
           sprintf(tmp_name, "%s/%s", tmp_dir, tmpnam_string);
-          _AL_FREE(tmpnam_string);
  
           if (tmp_name) {
  #ifndef ALLEGRO_MPW
--- 2004,2012 ----
           /* note: since the filename creation and the opening are not
            * an atomic operation, this is not secure
            */
!          tmpnam_string = tmpnam(NULL); /* Note: tmpnam_string now points to an internal buffer which does not need to be free()'d */
           tmp_name = _AL_MALLOC_ATOMIC(strlen(tmp_dir) + strlen(tmpnam_string) + 2);
           sprintf(tmp_name, "%s/%s", tmp_dir, tmpnam_string);
  
           if (tmp_name) {
  #ifndef ALLEGRO_MPW