[CF-Devel] Bugs in 1.3 with fixes

[Mark Wedel]

Kurt Fitzner wrote:
>
     
      Hi all,
     
     >
     
     
     
>
     
      2) new_save_map(), common/map.c
     
     >
     
      There is logic at the beginning of the function to determine if the map is a
     
     >
     
      temp map and not compress it if it is.  The logic is:
     
     >
     
     
     >
     
         if (m->compressed && (m->unique || flag))
     
     >
     
           /* popen() and compress it */
     
     >
     
         else
     
     >
     
           /* fopen() and don't compress it */
     
     >
     
     
     >
     
      There is logic at the end of the function to determine, again, if we have just
     
     >
     
      saved a temp map, and if so fclose() the file, if not, pclose().  This is that
     
     >
     
      logic:
     
     >
     
     
     >
     
          if (m->compressed && !flag)
     
     >
     
              pclose(fp);
     
     >
     
          else
     
     >
     
              fclose(fp);
     
     >
     
     
     >
     
      Seems as if the logic is reversed at the bottom.  The end result, is that temp
     
     >
     
      maps never get fclose()d.  On Linux this seems to not really matter, because
     
     >
     
      even open files get completely flushed periodically to disk.  On HPUX,
     
     >
     
      however, the last <=4k is not flushed unless an fflush() or fclose() is
     
     >
     
      called.  Even on Linux, though, you'll end up with gazillions of open files,
     
     >
     
      and this will leak memory and cause other problems.  I changed the ending
     
     >
     
      logic to match the opening, and it works well now.
     
     
  Yeah.  My guess is that it doesn't effect most people because most people 
never compress the map files - thus, the m->compressed is never set, so it 
always uses the fopen/fclose logic.

  Some good arguments can be made to get rid of the compress logic:
1) Simplifies code a bit.
2) Disk space availability has gone up a lot faster than the amount of space for 
maps - map distribution is currently under 50 MB - even maps-bigworld is only 
120 MB.
3) While cpu time has also gone up, the cost of compressing/decompressing maps 
could still perhaps cause some noticable glitches on the server, as the tick 
takes longer to complete than normal.

  OTOH, compressed map files take fewer blocks, so maybe the balance of not 
needing to read as much from the disk offsets the cpu issue.  However, at least 
for the main maps, since they are just written out once (at cvs checkout or 
untar), hopefully the files are on contigous blocks, so that the disk read 
should still be pretty quick.

>
     
     
     >
     
      3) get_empty_treasure(), common/treasure.c
     
     >
     
     
     >
     
      Here we malloc() a new treasure struct and initialize it.  Unfortunately, the
     
     >
     
      code doesn't initialize the change_arch struct.  On my HP this crashed the
     
     >
     
      server almost any time new treasure was created, since HPUX pointers are
     
     >
     
      virtual.  ix86 are virtual pointers too, but Intel goes through more pain to
     
     >
     
      make them look real, so if you have a garbage pointer in change_arch.name on a
     
     >
     
      Linux box, you'll happily end up with an object with a funny name.  On
     
     >
     
      the other hand, you might end up pointing to a point of memory with a megabyte
     
     >
     
      of non-null values and crash the game.  Or maybe Linux zeros out
     
     >
     
      malloc()ed memory.  Does someone know, for interest's sake? Anyway, Simply
     
     >
     
      initializing the change_arch struct's three pointers to NULL here might fix
     
     >
     
      nigling little crash bugs on Linux.  It makes it runnable on my HPUX.
     
     
  Yeah, that also seems broken - suppose it doesn't show up on other systems. 
Its very possible since all of those are allocated at the start of the run of 
the server, the processes heap needs to be increased in size, and the server 
does blank out that memory.  Security wise, the OS most clear the memory to a 
process before it gives it to the process (so that you can't get/see the data 
another process that has exited has allocated).  But there probably is no 
requirement that it gets cleared with 0's.  It could very well be getting 
cleared with other random values, which is what HPUX seems to do.

  At one time in crossfire history, there seemed to be some amount of work done 
to make initialization after a malloc only blank the 'important' fields.  This 
is perhaps understandable for structures with a lot of space, but probably 
better to use memset as it can use more clever methods for filling in a block of 
memory than just setting the values.  In any case, I've changed that 
get_treasure function to do a memset.


>
     
      I'm noticing that some (notably outdoor themed) random maps are showing up
     
     >
     
      populated with doors and monsters, but no background.  Don't know if I'm
     
     >
     
      missing maps or if there's a bug there.  If anyone knows and can save me time
     
     >
     
      chasing that one, I'd appreciate knowing.
     
     
  thats odd.  I'd make sure that you have a full set of styles maps.  But if you 
had no style maps, I would expect things to be really odd. But I just noticed 
that the maps-bigworld doesn't have the style maps, which if your using that map 
distribution could be the cause of oddness.