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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Jason Short wrote:
> Mike Kaufman wrote:
> 
>>On Wed, Mar 12, 2003 at 03:11:11PM -0800, Jason Short wrote:

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

Actually, I think that

(1) We need to more rigorously check the capstring length, particularly 
in init_our_capability().  One of these days (probably before the next 
release :-) we will overrun MAX_LEN_CAPSTR - and when we do it would be 
nice if things didn't just *silently* break.

(2) append_capability shouldn't export the problem by returning a 
boolean.  If the capstring does overflow it's because of a bug (that 
will affect everyone), and should be fixed (probably just by upping the 
protocol version #).

Patch attached.

jason

Index: client/civclient.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/civclient.c,v
retrieving revision 1.164
diff -u -r1.164 civclient.c
--- client/civclient.c  2003/02/22 13:22:32     1.164
+++ client/civclient.c  2003/03/21 23:06:34
@@ -196,6 +196,7 @@
   /* disallow running as root -- too dangerous */
   dont_run_as_root(argv[0], "freeciv_client");
 
+  init_our_capability();
   log_init(logfile, loglevel, NULL);
 
   /* after log_init: */
Index: common/capstr.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/capstr.c,v
retrieving revision 1.126
diff -u -r1.126 capstr.c
--- common/capstr.c     2003/02/17 22:49:27     1.126
+++ common/capstr.c     2003/03/21 23:06:35
@@ -19,6 +19,7 @@
 #include <stdlib.h>            /* getenv() */
 
 #include "connection.h"                /* MAX_LEN_CAPSTR */
+#include "shared.h"
 #include "support.h"
 
 #include "capstr.h"
@@ -119,5 +120,25 @@
   if (!s) {
     s = CAPABILITY;
   }
+
+  if (strlen(s) >= MAX_LEN_CAPSTR) {
+    die("Capstring too long: %s", s);
+  }
+
   sz_strlcpy(our_capability_internal, s);
+}
+
+/**************************************************************************
+  Append the string to the capability string.
+**************************************************************************/
+void append_capability(const char *str)
+{
+  size_t len;
+
+  sz_strlcat(our_capability_internal, " ");
+  len = mystrlcat(our_capability_internal, str, MAX_LEN_CAPSTR);
+
+  if (len >= MAX_LEN_CAPSTR) {
+    die("Appended capstring too long: %s", str);
+  }
 }
Index: common/capstr.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/capstr.h,v
retrieving revision 1.1
diff -u -r1.1 capstr.h
--- common/capstr.h     1999/06/10 12:22:15     1.1
+++ common/capstr.h     2003/03/21 23:06:35
@@ -16,5 +16,6 @@
 extern const char * const our_capability;
 
 void init_our_capability(void);
+void append_capability(const char *str);
 
 #endif
Index: common/game.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/game.c,v
retrieving revision 1.158
diff -u -r1.158 game.c
--- common/game.c       2003/02/20 09:45:21     1.158
+++ common/game.c       2003/03/21 23:06:36
@@ -738,7 +738,6 @@
   game.load_options.load_private_map = TRUE;
   game.load_options.load_settings = TRUE;
 
-  init_our_capability();    
   map_init();
   idex_init();
   
Index: server/srv_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/srv_main.c,v
retrieving revision 1.114
diff -u -r1.114 srv_main.c
--- server/srv_main.c   2003/02/12 22:22:35     1.114
+++ server/srv_main.c   2003/03/21 23:06:39
@@ -158,6 +158,7 @@
   init_nls();
 
   /* init server arguments... */
+  init_our_capability();
 
   srvarg.metaserver_no_send = DEFAULT_META_SERVER_NO_SEND;
   sz_strlcpy(srvarg.metaserver_info_line, default_meta_server_info_string());