Wireshark · Wireshark-dev: Re: [Wireshark-dev] g_snprintf() and sizeof

[Jakub Zawadzki <darkjames@xxxxxxxxxxxxxxxx>]

On Thu, Mar 19, 2009 at 08:06:55PM +0100, Jakub Zawadzki wrote:
> On Thu, Mar 19, 2009 at 11:12:03AM -0700, Guy Harris wrote:
> > Warning: g_snprintf()'s function signature has an annoying botch in it  
> > - the size argument is a gulong, not a gsize.
> > 
> > Not a problem in the UN*X and Windows ILP32 environment and in the  
> > UN*X LP64 environment, but it causes the Microsoft compiler to  
> > (correctly) warn about a conversion from a 64-bit integer to a 32-bit  
> > integer in the Windows LLP64 environment. 
> > Cast sizeof - or any other size_t value - to (gulong) before passing it 
> > as the length argument to g_snprintf().
> 
> What do you think about creating ws_snprintf() & ws_vsnprintf() macros,
> which would care about casting size to (gulong) ?

In attachment possible fixup, (I've also changed some g_snprintf() to g_strlcat())
Could you review?

Cheers

Index: wsutil/str_util.h
===================================================================
--- wsutil/str_util.h	(wersja 27821)
+++ wsutil/str_util.h	(kopia robocza)
@@ -25,6 +25,8 @@
 #ifndef __STR_UTIL_H__
 #define __STR_UTIL_H__
 
+#define ws_sncatprintf(str, size, ...) g_snprintf(str+strlen(str), (gulong) size-strlen(str), __VA_ARGS__)
+
 /** Convert all upper-case ASCII letters to their ASCII lower-case
  *  equivalents, in place, with a simple non-locale-dependent
  *  ASCII mapping (A-Z -> a-z).
Index: epan/dissectors/packet-rtps.c
===================================================================
--- epan/dissectors/packet-rtps.c	(wersja 27821)
+++ epan/dissectors/packet-rtps.c	(kopia robocza)
@@ -60,6 +60,7 @@
 #include  <glib.h>
 #include  <epan/packet.h>
 #include  <epan/addr_resolv.h>
+#include  <wsutil/str_util.h>
 
 
 #include "packet-rtps.h"
@@ -2624,8 +2625,7 @@
     dim_str[0] = '\0';
     for (i = 0; i < MAX_ARRAY_DIMENSION; ++i) {
       if (arr_dimension[i] != 0) {
-        g_snprintf(dim_str+strlen(dim_str),
-                   40-strlen(dim_str),
+        ws_sncatprintf(dim_str, sizeof(dim_str), 
                    "[%d]", arr_dimension[i]);
       } else {
         break;
@@ -4239,8 +4239,7 @@
         buffer[0] = '\0';
         while (param_length >= 4) {
           manager_key = NEXT_guint32(tvb, offset, little_endian);
-          g_snprintf(buffer+strlen(buffer),
-                        MAX_PARAM_SIZE-strlen(buffer),
+          ws_sncatprintf(buffer, MAX_PARAM_SIZE,
                         "%c 0x%08x",
                         sep,
                         manager_key);
@@ -5915,8 +5914,7 @@
         }
 /*
         if (smcr_ptr->counter > 1) {
-          g_snprintf(info_buf+strlen(info_buf), 
-                          MAX_SUMMARY_SIZE-strlen(info_buf), 
+          ws_sncatprintf(info_buf, MAX_SUMMARY_SIZE, 
                           "%s(%d)", 
                           val_to_str(smcr_ptr->id,
                                   submessage_id_vals, 
@@ -5924,17 +5922,14 @@
                           smcr_ptr->counter);
         } else {
 */
-          g_snprintf(info_buf+strlen(info_buf), 
-                          MAX_SUMMARY_SIZE-strlen(info_buf), 
+          ws_sncatprintf(info_buf, MAX_SUMMARY_SIZE, 
                           "%s%s", 
                           val_to_str(smcr_ptr->id,
                                   submessage_id_vals, 
                                   "Unknown[%02x]"),
                           smcr_ptr->extra ? smcr_ptr->extra : "");
           if (strlen(info_buf) > (MAX_SUMMARY_SIZE - 20)) {
-            g_snprintf(info_buf+strlen(info_buf),
-                        MAX_SUMMARY_SIZE-strlen(info_buf),
-                        "...");
+            g_strlcat(info_buf, "...", MAX_SUMMARY_SIZE);
             break;
           }
 /*
Index: epan/dissectors/packet-enttec.c
===================================================================
--- epan/dissectors/packet-enttec.c	(wersja 27821)
+++ epan/dissectors/packet-enttec.c	(kopia robocza)
@@ -39,6 +39,7 @@
 #include <epan/addr_resolv.h>
 #include <epan/prefs.h>
 #include <epan/strutil.h>
+#include <wsutil/str_util.h>
 
 /*
  * See
@@ -204,7 +205,6 @@
 	guint16 length,r,c,row_count;
 	guint8 v,type,count;
 	guint16 ci,ui,i,start_offset,end_offset;
-	char* ptr;
 
 	proto_tree_add_item(tree, hf_enttec_dmx_data_universe, tvb,
 					offset, 1, FALSE);
@@ -279,29 +279,23 @@
 		si = proto_item_add_subtree(hi, ett_enttec);
 			
 		row_count = (ui/global_disp_col_count) + ((ui%global_disp_col_count) == 0 ? 0 : 1);
-		ptr = string;
-		/* XX: In theory the g_snprintf statements below could store '\0' bytes off the end of the     */
-		/*     'string' buffer'. This is so since g_snprint returns the number of characters which     */
-		/*     "would have been written" (whether or not there was room) and since ptr is always       */
-		/*     incremented by this amount. In practice the string buffer is large enough such that the */
-		/*     string buffer size is not exceeded even with the maximum number of columns which might  */
-		/*     be displayed.                                                                           */
-		/*     ToDo: consider recoding slightly ...                                                    */
 		for (r=0; r < row_count;r++) {
+			string[0] = '\0';
+
 			for (c=0;(c < global_disp_col_count) && (((r*global_disp_col_count)+c) < ui);c++) {
 				if ((c % (global_disp_col_count/2)) == 0) {
-					ptr += g_snprintf(ptr, sizeof string - strlen(string), " ");
+					g_strlcat(string, " ", sizeof(string));
 				}
 				v = dmx_data[(r*global_disp_col_count)+c];
 				if (global_disp_chan_val_type == 0) {
 					v = (v * 100) / 255;
 					if (v == 100) {
-						ptr += g_snprintf(ptr, sizeof string - strlen(string), "FL ");
+						g_strlcat(string, "FL ", sizeof(string));
 					} else {
-						ptr += g_snprintf(ptr, sizeof string - strlen(string), chan_format[global_disp_chan_val_type], v);
+						ws_sncatprintf(string, sizeof(string), chan_format[global_disp_chan_val_type], v);
 					}
 				} else {
-					ptr += g_snprintf(ptr, sizeof string - strlen(string), chan_format[global_disp_chan_val_type], v);
+					ws_sncatprintf(string, sizeof(string), chan_format[global_disp_chan_val_type], v);
 				}
 			}
 
@@ -312,7 +306,6 @@
 						offset+start_offset, 
 						end_offset-start_offset,
 						string_format[global_disp_chan_nr_type], (r*global_disp_col_count)+1, string);
-			ptr = string;
 		}
 		
 		item = proto_tree_add_item(si, hf_enttec_dmx_data_data_filter, tvb,
Index: epan/dissectors/packet-rtps2.c
===================================================================
--- epan/dissectors/packet-rtps2.c	(wersja 27821)
+++ epan/dissectors/packet-rtps2.c	(kopia robocza)
@@ -56,6 +56,7 @@
 #include  <epan/packet.h>
 #include  <epan/addr_resolv.h>
 #include  <epan/prefs.h>
+#include  <wsutil/str_util.h>
 
 
 #include "packet-rtps2.h"
@@ -2898,8 +2899,7 @@
     dim_str[0] = '\0';
     for (i = 0; i < MAX_ARRAY_DIMENSION; ++i) {
       if (arr_dimension[i] != 0) {
-        g_snprintf(dim_str+strlen(dim_str),
-                   40-strlen(dim_str),
+        ws_sncatprintf(dim_str, sizeof(dim_str),
                    "[%d]", arr_dimension[i]);
       } else {
         break;
@@ -3698,10 +3698,10 @@
                         ett_rtps_entity,
                         "guidSuffix",
                         NULL);          
-        memset(buffer, 0, MAX_PARAM_SIZE);
+        buffer[0] = '\0';
         for (i = 0; i < 16; ++i) {
           guidPart = tvb_get_guint8(tvb, offset+i);
-          g_snprintf(buffer+strlen(buffer), MAX_PARAM_SIZE-strlen(buffer), 
+          ws_sncatprintf(buffer, MAX_PARAM_SIZE, 
                         "%02x", guidPart);
           if (i == 3 || i == 7 || i == 11) g_strlcat(buffer, ":", MAX_PARAM_SIZE);
         }
@@ -3730,7 +3730,7 @@
         g_strlcat(buffer, "guid: ", MAX_PARAM_SIZE);
         for (i = 0; i < param_length; ++i) {
           guidPart = tvb_get_guint8(tvb, offset+i);
-          g_snprintf(buffer+strlen(buffer), MAX_PARAM_SIZE-strlen(buffer), 
+          ws_sncatprintf(buffer, MAX_PARAM_SIZE, 
                         "%02x", guidPart);
           if (( ((i+1) % 4) == 0 ) && (i != param_length-1) ) 
             g_strlcat(buffer, ":", MAX_PARAM_SIZE);
@@ -5115,8 +5115,7 @@
         buffer[0] = '\0';
         while (param_length >= 4) {
           manager_key = NEXT_guint32(tvb, offset, little_endian);
-          g_snprintf(buffer+strlen(buffer),
-                        MAX_PARAM_SIZE-strlen(buffer),
+          ws_sncatprintf(buffer, MAX_PARAM_SIZE,
                         "%c 0x%08x",
                         sep,
                         manager_key);
Index: epan/dissectors/packet-pim.c
===================================================================
--- epan/dissectors/packet-pim.c	(wersja 27821)
+++ epan/dissectors/packet-pim.c	(kopia robocza)
@@ -36,6 +36,8 @@
 #include <epan/afn.h>
 #include "packet-ipv6.h"
 #include <epan/in_cksum.h>
+#include <wsutil/str_util.h>
+
 #include "packet-pim.h"
 
 #define PIM_TYPE(x)	((x) & 0x0f)
@@ -77,7 +79,7 @@
 	    flags_masklen & 0x0040 ? "R" : "");
     } else
 	buf[0] = '\0';
-	g_snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), "%s/%u",
+	ws_sncatprintf(buf, sizeof(buf), "%s/%u",
 	ip_to_str(tvb_get_ptr(tvb, offset + 2, 4)), flags_masklen & 0x3f);
 
     return buf;
@@ -566,7 +568,7 @@
 	    break;
 	}
 	if (flags) {
-	    g_snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf),
+	    ws_sncatprintf(buf, sizeof(buf),
 		" (%s%s%s)",
 		flags & 0x04 ? "S" : "",
 		flags & 0x02 ? "W" : "",
Index: epan/dissectors/packet-iec104.c
===================================================================
--- epan/dissectors/packet-iec104.c	(wersja 27821)
+++ epan/dissectors/packet-iec104.c	(kopia robocza)
@@ -37,6 +37,7 @@
 #include <epan/packet.h>
 #include <epan/dissectors/packet-tcp.h>
 #include <epan/emem.h>
+#include <wsutil/str_util.h>
 
 /* IEC-104 comment: Fields are little endian. */
 
@@ -474,11 +475,11 @@
 			Bytex = val_to_strlen(asduh->TNCause & F_CAUSE, causetx_types);
 			for (Ind=Bytex; Ind< 7; Ind++)   g_strlcat(res, " ", MAXS);
 		}
-		g_snprintf(res+strlen(res), MAXS-strlen(res), " IOA=%d", asduh->IOA);
+		ws_sncatprintf(res, MAXS, " IOA=%d", asduh->IOA);
 		if (asduh->NumIx > 1)   {
-			if (asduh->SQ == F_SQ)   g_snprintf(res+strlen(res), MAXS-strlen(res), "-%d", asduh->IOA+ asduh->NumIx- 1);
+			if (asduh->SQ == F_SQ)   ws_sncatprintf(res, MAXS, "-%d", asduh->IOA+ asduh->NumIx- 1);
 			else      g_strlcat(res, ",...", MAXS);
-			g_snprintf(res+strlen(res), MAXS-strlen(res), " (%u)", asduh->NumIx);
+			ws_sncatprintf(res, MAXS, " (%u)", asduh->NumIx);
 		}
 	}
 	else   {
@@ -587,24 +588,25 @@
 	if (Brossa != TcpLen)  {
 		if (apcih->ApduLen <= APDU_MAX_LEN)  {
 			/* APCI in 'Paquet List' */
-			g_snprintf(res+strlen(res), MAXS-strlen(res), "%s%s(", pinfo->srcport == iec104port ? "->" : "<-", val_to_str(apcih->Type, apci_types, "<ERR>"));
+			ws_sncatprintf(res, MAXS, "%s%s(", pinfo->srcport == iec104port ? "->" : "<-", val_to_str(apcih->Type, apci_types, "<ERR>"));
 			switch(apcih->Type) {  /* APCI in 'Packet List' */
 			case I_TYPE:
-				g_snprintf(res+strlen(res), MAXS-strlen(res), "%d,", apcih->Tx);
+				ws_sncatprintf(res, MAXS, "%d,", apcih->Tx);
 			case S_TYPE:
-				g_snprintf(res+strlen(res), MAXS-strlen(res), "%d)", apcih->Rx);
+				ws_sncatprintf(res, MAXS, "%d)", apcih->Rx);
 				/* Align first packets */
 				if (apcih->Tx < 10)  g_strlcat(res, " ", MAXS);
 				if (apcih->Rx < 10)  g_strlcat(res, " ", MAXS);
 				break;
 			case U_TYPE:
-				g_snprintf(res+strlen(res), MAXS-strlen(res), "%s)", val_to_str(apcih->UType >> 2, u_types, "<ERR>"));
+				ws_sncatprintf(res, MAXS, "%s)", val_to_str(apcih->UType >> 2, u_types, "<ERR>"));
 				break;
 			}
-			if (apcih->Type != I_TYPE  &&  apcih->ApduLen > APDU_MIN_LEN)   g_snprintf(res+strlen(res), MAXS-strlen(res), "<ERR %u bytes> ", apcih->ApduLen- APDU_MIN_LEN);
+			if (apcih->Type != I_TYPE  &&  apcih->ApduLen > APDU_MIN_LEN)
+				ws_sncatprintf(res, MAXS, "<ERR %u bytes> ", apcih->ApduLen- APDU_MIN_LEN);
 		}
 		else  {
-			g_snprintf(res+strlen(res), MAXS-strlen(res), "<ERR ApduLen=%u bytes> ", apcih->ApduLen);
+			ws_sncatprintf(res, MAXS, "<ERR ApduLen=%u bytes> ", apcih->ApduLen);
 		}
 	}
 	g_strlcat(res, " ", MAXS); /* We add an space to separate possible APCIs/ASDUs in the same packet */
Index: epan/dissectors/packet-artnet.c
===================================================================
--- epan/dissectors/packet-artnet.c	(wersja 27821)
+++ epan/dissectors/packet-artnet.c	(kopia robocza)
@@ -39,6 +39,7 @@
 #include <epan/addr_resolv.h>
 #include <epan/prefs.h>
 #include <epan/strutil.h>
+#include <wsutil/str_util.h>
 
 /*
  * See
@@ -757,7 +758,6 @@
   guint16 length,r,c,row_count;
   guint8 v;
   static char string[255];
-  char* ptr;
   const char* chan_format[] = {
     "%2u ",
     "%02x ",
@@ -795,37 +795,31 @@
   si = proto_item_add_subtree(hi, ett_artnet);
 
   row_count = (length/global_disp_col_count) + ((length%global_disp_col_count) == 0 ? 0 : 1);
-  ptr = string;
-  /* XX: In theory the g_snprintf statements below could store '\0' bytes off the end of the     */
-  /*     'string' buffer'. This is so since g_snprint returns the number of characters which     */
-  /*     "would have been written" (whether or not there was room) and since ptr is always       */
-  /*     incremented by this amount. In practice the string buffer is large enough such that the */
-  /*     string buffer size is not exceeded even with the maximum number of columns which might  */
-  /*     be displayed.                                                                           */
-  /*     ToDo: consider recoding slightly ...                                                    */
+
   for (r=0; r < row_count;r++) {
+    string[0] = '\0';
+
     for (c=0;(c < global_disp_col_count) && (((r*global_disp_col_count)+c) < length);c++) {
       if ((c % (global_disp_col_count/2)) == 0) {
-        ptr += g_snprintf(ptr, (gulong)(sizeof string - strlen(string)), " ");
+	 g_strlcat(string, " ", sizeof(string));
       }
 
       v = tvb_get_guint8(tvb, (offset+(r*global_disp_col_count)+c));
       if (global_disp_chan_val_type == 0) {
         v = (v * 100) / 255;
         if (v == 100) {
-          ptr += g_snprintf(ptr, (gulong)(sizeof string - strlen(string)), "FL ");
+          g_strlcat(string, "FL ", sizeof(string));
         } else {
-          ptr += g_snprintf(ptr, (gulong)(sizeof string - strlen(string)), chan_format[global_disp_chan_val_type], v);
+          ws_sncatprintf(string, sizeof(string), chan_format[global_disp_chan_val_type], v);
         }
       } else {
-        ptr += g_snprintf(ptr, (gulong)(sizeof string - strlen(string)), chan_format[global_disp_chan_val_type], v);
+        ws_sncatprintf(string, sizeof(string), chan_format[global_disp_chan_val_type], v);
       }
     }
     
     proto_tree_add_none_format(si,hf_artnet_output_dmx_data, tvb,
                                offset+(r*global_disp_col_count), c, 
                                string_format[global_disp_chan_nr_type], (r*global_disp_col_count)+1, string);
-    ptr = string;
   }
 
   /* Add the real type hidden */