[Freeciv-Dev] Re: (PR#3706) append the capstring

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Mike Kaufman wrote:
> On Wed, Mar 12, 2003 at 03:11:11PM -0800, Jason Short wrote:
> 
>>capability.c provides the code to handle capabilities; these are used 
>>for savegames, tilespec and specfiles, and network versioning.  capstr.c 
>>just provides the network versioning capstring.
> 
> 
> my point is why do we have two different files when one would do just
> fine...

The capstr.c code does not fit in to capability.c (any more than the 
savegame or tilespec capability code does).  It could be moved into some 
network file, though.

>>It does look like you don't handle the error case properly.  What 
>>happens if the capstring buffer is overflowed?  Will freeciv just break? 
>>  Even in the second patch, when you return FALSE to indicate error, it 
>>still leaves you with an incorrect capstring.
> 
> 
> mystrlcat prevents an buffer overflow, it simply cuts off the string.
> Freeciv won't break, the client simply won't recieve the looked-for string,
> and will probably act as if the capability doesn't exist.
> 
> This was meant to be a simple little patch. What would you propose as a
> solution?

The problem is it truncates the capability [1].  This isn't likely to 
break anything (at least within our lifetimes), but it is unsafe.

Either don't add anything to the capstring and return FALSE (this simply 
means checking the length before calling mystrlcat), or reallocate the 
capstring to be larger if necessary.

bool append_capability(const char *str)
{
   size_t len;

   len = strlen(our_capability_internal) + strlen(str) + strlen(" ");
   if (len >= MAX_LEN_CAPSTR) return FALSE;

   sz_strlcat(our_capability_internal, " ");
   len = mystrlcat(our_capability_internal, str, MAX_LEN_CAPSTR);

   assert(len < MAX_LEN_CAPSTR);
   return TRUE;
}

jason