Re: [XBoard-devel] Buffer overflow

[h.g. muller]

At 20:22 22-12-2011 +0100, Byrial Jensen wrote:
> I think that the function PGNTagsStatic() in pgntags.c is unsafe. The 
> problem is that gameInfo->extraTags which can have any size is copied 
> without size control to a buffer which can overflow as the result. This is 
> rather serious as it is easy to construct a pgn file that can cause the 
> overflow.
>
> You cannot have something of possibly unlimited size in a static buffer, 
> so I suggest that PGNTagsStatic is removed. PGNTags() can then allocate a 
> big eneogh buffer and do the work itself. PrintPGNTags() doesn't need a 
> buffer, but can print directly to the file.

I think I was responsible for this, because the problem was that the 
original malloc'ed code caused a devastating memory leak in WinBoard on my 
Win XP laptop. Although I could not find anything wrong with the code; it 
seemedthe free() was just not working. But it led to WB crashing when you 
loaded a couple of large PGN files, (~40k games a piece), because it could 
no longer allocate memory. So I made it static based on the philosophy that 
it is better to have a hypothetical problem that never ocurs in practice, 
than having a certain crash on commonly done tasks...


> The "guilty" function is strcat() which is used in many places, so it is 
> probably a good idea to check all uses of strcat.
>
> However there is no reason to replace all occurences of strcat with 
> something else like it is done with strcpy(). I find it a little silly to 
> see calls to safeStrCpy instead of strcpy when you have just allocated a 
> new buffer of the required size, so strcpy would be perfectly safe to use.

True. This whole safeStrCpy business is a folly IMO, and it has already 
caused a lot of grief. But it seems we cannot build for some distributions 
when we use strcpy, because it is a blacklisted function. I already 
proposed to subvert this ridiculous demand by simply ading a "#define 
strcpy myStrCpy" in common.h, and providing our own version of strcpy not 
detected by these compilers, but Arun wouldn't have it!