Fix some warnings reported by CODESonar

Remove some unreached/duplicated code. Add error checking for `atoi()` calls. About `isdigit()` and similar functions. The warning reported is: ``` Negative Character Value help isdigit() is invoked here with an argument of signed type char, but only has defined behavior for int arguments that are either representable as unsigned char or equal to the value of macro EOF(-1). Casting the argument to unsigned char will avoid the undefined behavior. In a number of libc implementations, isdigit() is implemented using lookup tables (arrays): passing in a negative value can result in a read underrun. ``` Switching to our macros fix that
ntop · Dec 22, 2023 · 0dc75e6 · 0dc75e6
1 parent 5eb468d
commit 0dc75e6
Show file tree

Hide file tree

Showing 15 changed files with 55 additions and 63 deletions.
diff --git a/src/lib/ndpi_analyze.c b/src/lib/ndpi_analyze.c
@@ -134,7 +134,7 @@ float ndpi_data_average(struct ndpi_analyze_struct *s) {
   if((!s) || (s->num_data_entries == 0))
     return(0);
 
-  return((s->num_data_entries == 0) ? 0 : ((float)s->sum_total / (float)s->num_data_entries));
+  return((float)s->sum_total / (float)s->num_data_entries);
 }
 
 /* ********************************************************************************* */

diff --git a/src/lib/ndpi_classify.c b/src/lib/ndpi_classify.c
@@ -549,9 +549,9 @@ ndpi_classify (const unsigned short *pkt_len, const pkt_timeval *pkt_time,
 void
 ndpi_update_params (classifier_type_codes_t param_type, const char *param_file)
 {
-  float param;
+  float param = 0.0;
   FILE *fp;
-  int count = 0;
+  int count;
 
   switch (param_type) {
   case (SPLT_PARAM_TYPE):
@@ -691,7 +691,7 @@ ndpi_log_timestamp(char *log_ts, uint32_t log_ts_len)
 {
   pkt_timeval tv;
   time_t nowtime;
-  struct tm nowtm_r;
+  struct tm nowtm_r = { 0 };
   char tmbuf[NDPI_TIMESTAMP_LEN];
 
   gettimeofday(&tv, NULL);

diff --git a/src/lib/ndpi_domain_classify.c b/src/lib/ndpi_domain_classify.c
@@ -224,7 +224,7 @@ bool ndpi_domain_classify_contains(ndpi_domain_classify *s,
   if((!strcmp(dot, ".arpa")) || (!strcmp(dot, ".local"))) return(false);
 
   /* This is a number or a numeric IP or similar */
-  if(isdigit(domain[len-1]) && isdigit(domain[0])) {
+  if(ndpi_isdigit(domain[len-1]) && isdigit(domain[0])) {
 #ifdef DEBUG_CONTAINS
     printf("[contains] %s INVALID\n", domain);
 #endif

diff --git a/src/lib/ndpi_main.c b/src/lib/ndpi_main.c
@@ -4513,7 +4513,7 @@ int ndpi_load_categories_file(struct ndpi_detection_module_struct *ndpi_str,
 int load_categories_file_fd(struct ndpi_detection_module_struct *ndpi_str,
 			    FILE *fd, void *user_data) {
   char buffer[512], *line, *name, *category, *saveptr;
-  int len, num = 0;
+  int len, num = 0, cat_id;
 
   if(!ndpi_str || !fd)
     return(-1);
@@ -4536,12 +4536,16 @@ int load_categories_file_fd(struct ndpi_detection_module_struct *ndpi_str,
       category = strtok_r(NULL, "\t", &saveptr);
 
       if(category) {
-	int rc = ndpi_load_category(ndpi_str, name,
-				    (ndpi_protocol_category_t) atoi(category),
-				    user_data);
+        cat_id = atoi(category);
+        if(cat_id > 0 && cat_id < NDPI_PROTOCOL_NUM_CATEGORIES) {
+
+          int rc = ndpi_load_category(ndpi_str, name,
+				      (ndpi_protocol_category_t)cat_id,
+				      user_data);
 
-	if(rc >= 0)
-	  num++;
+          if(rc >= 0)
+           num++;
+        }
       }
     }
   }
@@ -4608,7 +4612,7 @@ int ndpi_load_category_file(struct ndpi_detection_module_struct *ndpi_str,
         line[i] = '\0';
         break;
       }
-      if (line[i] != '-' && line[i] != '.' && isalnum(line[i]) == 0
+      if (line[i] != '-' && line[i] != '.' && ndpi_isalnum(line[i]) == 0
           /* non standard checks for the sake of compatibility */
           && line[i] != '_')
         break;
@@ -4663,19 +4667,19 @@ int ndpi_load_categories_dir(struct ndpi_detection_module_struct *ndpi_str,
 
     /* Check if the format is <proto it>_<string>.<extension> */
     if((underscore = strchr(dp->d_name, '_')) != NULL) {
-      ndpi_protocol_category_t proto_id;
+      int cat_id;
 
       underscore[0] = '\0';
-      proto_id = (ndpi_protocol_category_t)atoi(dp->d_name);
+      cat_id = atoi(dp->d_name);
 
-      if((proto_id > 0) && (proto_id < (u_int16_t)NDPI_LAST_IMPLEMENTED_PROTOCOL)) {
+      if(cat_id > 0 && cat_id < NDPI_PROTOCOL_NUM_CATEGORIES) {
 	/* Valid file */
 	char path[512];
 
 	underscore[0] = '_';
 	snprintf(path, sizeof(path), "%s/%s", dir_path, dp->d_name);
 
-	if (ndpi_load_category_file(ndpi_str, path, proto_id) < 0) {
+	if (ndpi_load_category_file(ndpi_str, path, (ndpi_protocol_category_t)cat_id) < 0) {
 	  NDPI_LOG_ERR(ndpi_str, "Failed to load '%s'\n", path);
 	  failed_files++;
 	}else
@@ -5931,9 +5935,6 @@ int ndpi_handle_ipv6_extension_headers(u_int16_t l3len, const u_int8_t **l4ptr,
 
     *nxt_hdr = (*l4ptr)[0];
 
-    if(*l4len < ehdr_len)
-      return(1);
-
     *l4len -= ehdr_len;
     (*l4ptr) += ehdr_len;
   }
@@ -5989,8 +5990,7 @@ static u_int8_t ndpi_detection_get_l4_internal(struct ndpi_detection_module_stru
   if(l3 == NULL || l3_len < sizeof(struct ndpi_iphdr))
     return(1);
 
-  if((iph = (const struct ndpi_iphdr *) l3) == NULL)
-    return(1);
+  iph = (const struct ndpi_iphdr *) l3;
 
   if(iph->version == IPVERSION && iph->ihl >= 5) {
     NDPI_LOG_DBG2(ndpi_str, "ipv4 header\n");
@@ -6975,7 +6975,7 @@ static void ndpi_reconcile_protocols(struct ndpi_detection_module_struct *ndpi_s
       (MS Teams uses Skype as transport protocol for voice/video)
     */
   case NDPI_PROTOCOL_MSTEAMS:
-    if(flow && (flow->l4_proto == IPPROTO_TCP)) {
+    if(flow->l4_proto == IPPROTO_TCP) {
       // printf("====>> NDPI_PROTOCOL_MSTEAMS\n");
 
       if(ndpi_str->msteams_cache)
@@ -6987,7 +6987,7 @@ static void ndpi_reconcile_protocols(struct ndpi_detection_module_struct *ndpi_s
     break;
 
   case NDPI_PROTOCOL_STUN:
-    if(flow && (flow->guessed_protocol_id_by_ip == NDPI_PROTOCOL_MICROSOFT_AZURE))
+    if(flow->guessed_protocol_id_by_ip == NDPI_PROTOCOL_MICROSOFT_AZURE)
       ndpi_reconcile_msteams_udp(ndpi_str, flow, NDPI_PROTOCOL_STUN);
     break;
 
@@ -7010,8 +7010,7 @@ static void ndpi_reconcile_protocols(struct ndpi_detection_module_struct *ndpi_s
       When Teams is unable to communicate via UDP
       it switches to TLS.TCP. Let's try to catch it
     */
-    if(flow
-       && (flow->guessed_protocol_id_by_ip == NDPI_PROTOCOL_MICROSOFT_AZURE)
+    if((flow->guessed_protocol_id_by_ip == NDPI_PROTOCOL_MICROSOFT_AZURE)
        && (ret->master_protocol == NDPI_PROTOCOL_UNKNOWN)
        && ndpi_str->msteams_cache
       ) {
@@ -9037,7 +9036,7 @@ const char *ndpi_category_get_name(struct ndpi_detection_module_struct *ndpi_str
   }
 
   if((category >= NDPI_PROTOCOL_CATEGORY_CUSTOM_1) && (category <= NDPI_PROTOCOL_CATEGORY_CUSTOM_5)) {
-    switch(category) {
+    switch((int)category) {
     case NDPI_PROTOCOL_CATEGORY_CUSTOM_1:
       return(ndpi_str->custom_category_labels[0]);
     case NDPI_PROTOCOL_CATEGORY_CUSTOM_2:
@@ -9048,13 +9047,9 @@ const char *ndpi_category_get_name(struct ndpi_detection_module_struct *ndpi_str
       return(ndpi_str->custom_category_labels[3]);
     case NDPI_PROTOCOL_CATEGORY_CUSTOM_5:
       return(ndpi_str->custom_category_labels[4]);
-    case NDPI_PROTOCOL_NUM_CATEGORIES:
-      return("Code should not use this internal constant");
-    default:
-      return("Unspecified");
     }
-  } else
-    return(categories[category]);
+  }
+  return(categories[category]);
 }
 
 /* ****************************************************** */
@@ -10124,7 +10119,7 @@ u_int8_t ends_with(struct ndpi_detection_module_struct *ndpi_struct,
 /* ******************************************************************** */
 
 static int ndpi_is_trigram_char(char c) {
-  if(isdigit(c) || (c == '.') || (c == '-'))
+  if(ndpi_isdigit(c) || (c == '.') || (c == '-'))
     return(0);
   else
     return(1);
@@ -10188,7 +10183,7 @@ int ndpi_check_dga_name(struct ndpi_detection_module_struct *ndpi_str,
        ndpi_match_string_subprotocol(ndpi_str, name, strlen(name), &ret_match) > 0)
       return(0); /* Ignore DGA for known domain names */
 
-    if(isdigit((int)name[0])) {
+    if(ndpi_isdigit(name[0])) {
       struct in_addr ip_addr;
 
       ip_addr.s_addr = inet_addr(name);
@@ -10224,7 +10219,7 @@ int ndpi_check_dga_name(struct ndpi_detection_module_struct *ndpi_str,
 	if(tmp[j] == '.') {
 	  num_dots++;
 	} else if(num_dots == 0) {
-	  if(!isdigit((int)tmp[j]))
+	  if(!ndpi_isdigit(tmp[j]))
 	    first_element_is_numeric = 0;
 	}
 
@@ -10237,10 +10232,10 @@ int ndpi_check_dga_name(struct ndpi_detection_module_struct *ndpi_str,
 	} else
 	  num_char_repetitions = 1, last_char = tmp[j];
 
-	if(isdigit((int)tmp[j])) {
+	if(ndpi_isdigit(tmp[j])) {
 	  num_digits++;
 
-	  if(((j+2)<(u_int)len) && isdigit((int)tmp[j+1]) && (tmp[j+2] == '.')) {
+	  if(((j+2)<(u_int)len) && ndpi_isdigit(tmp[j+1]) && (tmp[j+2] == '.')) {
 	    /* Check if there are too many digits */
 	    if(num_digits < 4)
 	      return(0); /* Double digits */
@@ -10339,7 +10334,7 @@ int ndpi_check_dga_name(struct ndpi_detection_module_struct *ndpi_str,
 	trigram_char_skip = 0;
 
 	for(i = 0; word[i+1] != '\0'; i++) {
-	  if(isdigit((int)word[i]))
+	  if(ndpi_isdigit(word[i]))
 	    num_consecutive_digits++;
 	  else {
 	    if((num_word == 1) && (num_consecutive_digits > max_num_consecutive_digits_first_word))

diff --git a/src/lib/ndpi_serializer.c b/src/lib/ndpi_serializer.c
@@ -74,7 +74,7 @@ static int ndpi_is_number(const char *str, u_int32_t str_len) {
   unsigned int i;
 
   for(i = 0; i < str_len; i++)
-    if(!isdigit((int)str[i])) return(0);
+    if(!ndpi_isdigit(str[i])) return(0);
 
   return(1);
 }

diff --git a/src/lib/ndpi_utils.c b/src/lib/ndpi_utils.c
@@ -696,7 +696,7 @@ static inline int ndpi_is_other_char(char c) {
 /* ******************************************************************** */
 
 static int _ndpi_is_valid_char(char c) {
-  if(ispunct(c) && (!ndpi_is_other_char(c)))
+  if(ndpi_ispunct(c) && (!ndpi_is_other_char(c)))
     return(0);
   else
     return(ndpi_isdigit(c)
@@ -722,7 +722,7 @@ static int ndpi_find_non_eng_bigrams(struct ndpi_detection_module_struct *ndpi_s
 				     char *str) {
   char s[3];
 
-  if((isdigit((int)str[0]) && isdigit((int)str[1]))
+  if((ndpi_isdigit(str[0]) && ndpi_isdigit(str[1]))
      || ndpi_is_other_char(str[0])
      || ndpi_is_other_char(str[1])
      )
@@ -741,7 +741,7 @@ int ndpi_has_human_readeable_string(struct ndpi_detection_module_struct *ndpi_st
 				    char *buffer, u_int buffer_size,
 				    u_int8_t min_string_match_len,
 				    char *outbuf, u_int outbuf_len) {
-  u_int ret = 0, i = 0, do_cr = 0, len = 0, o_idx = 0, being_o_idx = 0;
+  u_int ret = 0, i, do_cr = 0, len = 0, o_idx = 0, being_o_idx = 0;
 
   if(buffer_size <= 0)
     return(0);

diff --git a/src/lib/protocols/checkmk.c b/src/lib/protocols/checkmk.c
@@ -60,7 +60,7 @@ static void ndpi_search_checkmk(struct ndpi_detection_module_struct *ndpi_struct
      * this will detect the OpenSession command of the Data Stream Interface (DSI) protocol
      * which is exclusively used by the Apple Filing Protocol (AFP) on TCP/IP networks
      */
-    if (packet->payload_packet_len >= 15 && packet->payload_packet_len < 100
+    if (packet->payload_packet_len < 100
         && memcmp(packet->payload, "<<<check_mk>>>", 14) == 0) {
 
       NDPI_LOG_DBG(ndpi_struct, "Check_MK: Flow detected.\n");

diff --git a/src/lib/protocols/http.c b/src/lib/protocols/http.c
@@ -122,7 +122,7 @@ static int ndpi_search_http_tcp_again(struct ndpi_detection_module_struct *ndpi_
 /* *********************************************** */
 
 static int ndpi_http_is_print(char c) {
-  if(isprint(c) || (c == '\t') || (c == '\r') || (c == '\n'))
+  if(ndpi_isprint(c) || (c == '\t') || (c == '\r') || (c == '\n'))
     return(1);
   else
     return(0);
@@ -568,11 +568,11 @@ static void ndpi_check_user_agent(struct ndpi_detection_module_struct *ndpi_stru
        * We assume at least one non alpha char.
        * e.g. ' ', '-' or ';' ...
        */
-      if (isalpha(ua[i]) == 0)
+      if (ndpi_isalpha(ua[i]) == 0)
       {
         break;
       }
-      if (isupper(ua[i]) != 0)
+      if (isupper((unsigned char)ua[i]) != 0)
       {
         upper_case_count++;
       }
@@ -771,7 +771,7 @@ static void ndpi_check_http_server(struct ndpi_detection_module_struct *ndpi_str
 	char buf[16] = { '\0' };
 
 	for(i=off, j=0; (i<server_len) && (j<sizeof(buf)-1)
-	      && (isdigit(server[i]) || (server[i] == '.')); i++)
+	      && (ndpi_isdigit(server[i]) || (server[i] == '.')); i++)
 	  buf[j++] = server[i];
 
 	if(sscanf(buf, "%d.%d.%d", &a, &b, &c) == 3) {
@@ -816,7 +816,7 @@ static void check_content_type_and_change_protocol(struct ndpi_detection_module_
      && (packet->host_line.len > 0)) {
     int len = packet->http_url_name.len + packet->host_line.len + 1;
 
-    if(isdigit(packet->host_line.ptr[0])
+    if(ndpi_isdigit(packet->host_line.ptr[0])
        && (packet->host_line.len < 21))
       ndpi_check_numeric_ip(ndpi_struct, flow, (char*)packet->host_line.ptr, packet->host_line.len);
 

diff --git a/src/lib/protocols/mqtt.c b/src/lib/protocols/mqtt.c
@@ -180,18 +180,12 @@ static void ndpi_search_mqtt(struct ndpi_detection_module_struct *ndpi_struct,
 	if (pt == PUBLISH) {
 		// payload CAN be zero bytes length (section 3.3.3 of MQTT standard)
 		u_int8_t qos = (u_int8_t) (flags & 0x06);
-		u_int8_t retain = (u_int8_t) (flags & 0x01);
 		u_int8_t dup = (u_int8_t) (flags & 0x04);
 		if (qos > 2) { // qos values possible are 0,1,2
 			NDPI_LOG_DBG(ndpi_struct, "Excluding Mqtt invalid PUBLISH qos\n");
 			NDPI_ADD_PROTOCOL_TO_BITMASK(flow->excluded_protocol_bitmask, NDPI_PROTOCOL_MQTT);
 			return;
 		}
-		if (retain > 1) { // retain flag possible 0,1
-			NDPI_LOG_DBG(ndpi_struct, "Excluding Mqtt invalid PUBLISH retain\n");
-			NDPI_ADD_PROTOCOL_TO_BITMASK(flow->excluded_protocol_bitmask, NDPI_PROTOCOL_MQTT);
-			return;
-		}
 		if (dup > 1) { // dup flag possible 0,1
 			NDPI_LOG_DBG(ndpi_struct, "Excluding Mqtt invalid PUBLISH dup\n");
 			NDPI_ADD_PROTOCOL_TO_BITMASK(flow->excluded_protocol_bitmask, NDPI_PROTOCOL_MQTT);

diff --git a/src/lib/protocols/protobuf.c b/src/lib/protocols/protobuf.c
@@ -78,7 +78,7 @@ protobuf_dissect_varint(struct ndpi_packet_struct const * const packet,
   size_t i;
   *value = 0;
 
-  for (i = 0; i < 9; ++i)
+  for (i = 0; i < 10; ++i)
   {
     if (packet->payload_packet_len < *offset + i + 1)
     {

diff --git a/src/lib/protocols/tcp_udp.c b/src/lib/protocols/tcp_udp.c
@@ -56,8 +56,7 @@ void ndpi_search_tcp_or_udp(struct ndpi_detection_module_struct *ndpi_struct, st
   if(packet->iph /* IPv4 Only: we need to support packet->iphv6 at some point */) {
     proto = ndpi_search_tcp_or_udp_raw(ndpi_struct,
 				       flow,
-				       packet->iph ? packet->iph->protocol :
-				       packet->iphv6->ip6_hdr.ip6_un1_nxt,
+				       flow->l4_proto,
 				       ntohl(packet->iph->saddr), 
 				       ntohl(packet->iph->daddr));
 

diff --git a/src/lib/protocols/telegram.c b/src/lib/protocols/telegram.c
@@ -75,7 +75,7 @@ static void ndpi_search_telegram(struct ndpi_detection_module_struct *ndpi_struc
       u_int16_t sport = ntohs(packet->udp->source), dport = ntohs(packet->udp->dest);
 
       if(is_telegram_port_range(sport) || is_telegram_port_range(dport)) {
-	u_int i=0, found = 0;
+	u_int i, found = 0;
 
 	for(i=0; i<packet->payload_packet_len; i++) {
 	  if(packet->payload[i] == 0xFF) {

diff --git a/src/lib/third_party/src/gcrypt_light.c b/src/lib/third_party/src/gcrypt_light.c
@@ -344,8 +344,14 @@ static gcry_error_t _gcry_cipher_crypt (gcry_cipher_hd_t h,
                         src ? src:(const unsigned char *)in, (unsigned char *)out);
             break;
         case GCRY_CIPHER_MODE_GCM:
-            if(encrypt) return MBEDTLS_ERR_GCM_NOT_SUPPORT;
-            if(!( h->s_key && h->s_auth && h->s_iv && !h->s_crypt_ok)) return MBEDTLS_ERR_GCM_MISSING_KEY;
+            if(encrypt) {
+                ndpi_free(src);
+                return MBEDTLS_ERR_GCM_NOT_SUPPORT;
+            }
+            if(!( h->s_key && h->s_auth && h->s_iv && !h->s_crypt_ok)) {
+                ndpi_free(src);
+                return MBEDTLS_ERR_GCM_MISSING_KEY;
+            }
             h->taglen = 16;
             rv = mbedtls_gcm_crypt_and_tag(h->ctx.gcm,
                         MBEDTLS_GCM_DECRYPT,

diff --git a/src/lib/third_party/src/ndpi_patricia.c b/src/lib/third_party/src/ndpi_patricia.c
@@ -76,8 +76,6 @@ static u_char* ndpi_prefix_tochar (ndpi_prefix_t * prefix)
   if(family == AF_INET) return ((u_char *) & prefix->add.sin);
   else if(family == AF_INET6) return ((u_char *) & prefix->add.sin6);
   else /* if(family == AF_MAC) */ return ((u_char *) & prefix->add.mac);
-
-  return ((u_char *) & prefix->add.sin);
 }
 
 static int ndpi_comp_with_mask (void *addr, void *dest, u_int mask) {

diff --git a/tests/cfgs/default/result/threema.pcap.out b/tests/cfgs/default/result/threema.pcap.out
@@ -3,7 +3,7 @@ Guessed flow protos:	2
 DPI Packets (TCP):	66	(11.00 pkts/flow)
 Confidence DPI              : 4 (flows)
 Confidence Match by IP      : 2 (flows)
-Num dissector calls: 1308 (218.00 diss/flow)
+Num dissector calls: 1311 (218.50 diss/flow)
 LRU cache ookla:      0/0/0 (insert/search/found)
 LRU cache bittorrent: 0/6/0 (insert/search/found)
 LRU cache zoom:       0/0/0 (insert/search/found)