[LEDE-DEV,2/2] firewall3: Fix errors reported by cppcheck.

Message ID 1514242524-4319-2-git-send-email-rosenp@gmail.com
State Rejected
Headers show
Series
  • [LEDE-DEV,1/2] firewall3: Replace strerror(errno) with %m
Related show

Commit Message

Rosen Penev Dec. 25, 2017, 10:55 p.m.
Mainly sign conversion errors with printf function (%d vs. %u). Exact command line used was:

cppcheck --enable=all --inconclusive --force . 2> err.txt && cat err.txt | grep -v never | grep -v reduced

Only errors that I felt comfortable with were fixed.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 defaults.c |  2 +-
 ipsets.c   |  2 +-
 iptables.c | 11 ++++++-----
 options.c  |  2 +-
 ubus.c     |  4 ++--
 5 files changed, 11 insertions(+), 10 deletions(-)

Comments

Arjen de Korte Dec. 26, 2017, 12:30 p.m. | #1
Citeren Rosen Penev <rosenp@gmail.com>:

> Mainly sign conversion errors with printf function (%d vs. %u).  
> Exact command line used was:
>
> cppcheck --enable=all --inconclusive --force . 2> err.txt && cat  
> err.txt | grep -v never | grep -v reduced
>
> Only errors that I felt comfortable with were fixed.

Generally, NACK to this one. Unless you can provide an instance where  
this actually fixes a bug, I have seen these kind of changes break  
things in unexpected ways. For instance, if the output is somehow  
parsed by an external program, comparing (unsigned)-1 and (signed)-1  
against an arbitrary value may result in different outcomes.

     (unsigned)-1 is greater than 100
     (signed)-1 is smaller than 100

Code relying on the above is bug-ugly, but fixing compiler warnings is  
certainly not worth taking any risk here, unless one can demonstrate  
it fixes an actual problem.

> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  defaults.c |  2 +-
>  ipsets.c   |  2 +-
>  iptables.c | 11 ++++++-----
>  options.c  |  2 +-
>  ubus.c     |  4 ++--
>  5 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/defaults.c b/defaults.c
> index f1be1dd..41c41a4 100644
> --- a/defaults.c
> +++ b/defaults.c
> @@ -331,7 +331,7 @@ set_default(const char *name, int set)
>  		return;
>  	}
>
> -	fprintf(f, "%u\n", set);
> +	fprintf(f, "%d\n", set);
>  	fclose(f);
>  }
>
> diff --git a/ipsets.c b/ipsets.c
> index 30c6463..9ce89aa 100644
> --- a/ipsets.c
> +++ b/ipsets.c
> @@ -321,7 +321,7 @@ fw3_load_ipsets(struct fw3_state *state, struct  
> uci_package *p,
>  static void
>  create_ipset(struct fw3_ipset *ipset, struct fw3_state *state)
>  {
> -	bool first = true;
> +	bool first;
>
>  	struct fw3_ipset_datatype *type;
>
> diff --git a/iptables.c b/iptables.c
> index d848239..70bfa59 100644
> --- a/iptables.c
> +++ b/iptables.c
> @@ -929,11 +929,12 @@ void
>  fw3_ipt_rule_mac(struct fw3_ipt_rule *r, struct fw3_mac *mac)
>  {
>  	char buf[sizeof("ff:ff:ff:ff:ff:ff\0")];
> -	uint8_t *addr = mac->mac.ether_addr_octet;
> +	uint8_t *addr;
>
>  	if (!mac)
>  		return;
>
> +	addr = mac->mac.ether_addr_octet;
>  	sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
>  	        addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
>
> @@ -981,12 +982,12 @@ fw3_ipt_rule_limit(struct fw3_ipt_rule *r,  
> struct fw3_limit *limit)
>
>  	fw3_ipt_rule_addarg(r, false, "-m", "limit");
>
> -	sprintf(buf, "%u/%s", limit->rate, fw3_limit_units[limit->unit]);
> +	sprintf(buf, "%d/%s", limit->rate, fw3_limit_units[limit->unit]);
>  	fw3_ipt_rule_addarg(r, limit->invert, "--limit", buf);
>
>  	if (limit->burst > 0)
>  	{
> -		sprintf(buf, "%u", limit->burst);
> +		sprintf(buf, "%d", limit->burst);
>  		fw3_ipt_rule_addarg(r, limit->invert, "--limit-burst", buf);
>  	}
>  }
> @@ -1089,7 +1090,7 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r,  
> struct fw3_time *time)
>  				if (p > buf)
>  					*p++ = ',';
>
> -				p += sprintf(p, "%u", i);
> +				p += sprintf(p, "%d", i);
>  			}
>  		}
>
> @@ -1105,7 +1106,7 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r,  
> struct fw3_time *time)
>  				if (p > buf)
>  					*p++ = ',';
>
> -				p += sprintf(p, "%u", i);
> +				p += sprintf(p, "%d", i);
>  			}
>  		}
>
> diff --git a/options.c b/options.c
> index f686cf0..8b2a8fd 100644
> --- a/options.c
> +++ b/options.c
> @@ -1145,7 +1145,7 @@ fw3_address_to_string(struct fw3_address  
> *address, bool allow_invert, bool as_ci
>  	}
>  	else
>  	{
> -		p += sprintf(p, "/%u", fw3_netmask2bitlen(address->family,
> +		p += sprintf(p, "/%d", fw3_netmask2bitlen(address->family,
>  		                                          &address->mask.v6));
>  	}
>
> diff --git a/ubus.c b/ubus.c
> index 5bb4f5d..1c823f1 100644
> --- a/ubus.c
> +++ b/ubus.c
> @@ -260,10 +260,10 @@ static void fw3_ubus_rules_add(struct blob_buf  
> *b, const char *service,
>  	}
>
>  	if (instance)
> -		snprintf(comment, sizeof(comment), "ubus:%s[%s] %s %d",
> +		snprintf(comment, sizeof(comment), "ubus:%s[%s] %s %u",
>  				service, instance, type ? type : "rule", n);
>  	else
> -		snprintf(comment, sizeof(comment), "ubus:%s %s %d",
> +		snprintf(comment, sizeof(comment), "ubus:%s %s %u",
>  				service, type ? type : "rule", n);
>
>  	blobmsg_add_string(b, "name", comment);

Patch

diff --git a/defaults.c b/defaults.c
index f1be1dd..41c41a4 100644
--- a/defaults.c
+++ b/defaults.c
@@ -331,7 +331,7 @@  set_default(const char *name, int set)
 		return;
 	}
 
-	fprintf(f, "%u\n", set);
+	fprintf(f, "%d\n", set);
 	fclose(f);
 }
 
diff --git a/ipsets.c b/ipsets.c
index 30c6463..9ce89aa 100644
--- a/ipsets.c
+++ b/ipsets.c
@@ -321,7 +321,7 @@  fw3_load_ipsets(struct fw3_state *state, struct uci_package *p,
 static void
 create_ipset(struct fw3_ipset *ipset, struct fw3_state *state)
 {
-	bool first = true;
+	bool first;
 
 	struct fw3_ipset_datatype *type;
 
diff --git a/iptables.c b/iptables.c
index d848239..70bfa59 100644
--- a/iptables.c
+++ b/iptables.c
@@ -929,11 +929,12 @@  void
 fw3_ipt_rule_mac(struct fw3_ipt_rule *r, struct fw3_mac *mac)
 {
 	char buf[sizeof("ff:ff:ff:ff:ff:ff\0")];
-	uint8_t *addr = mac->mac.ether_addr_octet;
+	uint8_t *addr;
 
 	if (!mac)
 		return;
 
+	addr = mac->mac.ether_addr_octet;
 	sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
 	        addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
 
@@ -981,12 +982,12 @@  fw3_ipt_rule_limit(struct fw3_ipt_rule *r, struct fw3_limit *limit)
 
 	fw3_ipt_rule_addarg(r, false, "-m", "limit");
 
-	sprintf(buf, "%u/%s", limit->rate, fw3_limit_units[limit->unit]);
+	sprintf(buf, "%d/%s", limit->rate, fw3_limit_units[limit->unit]);
 	fw3_ipt_rule_addarg(r, limit->invert, "--limit", buf);
 
 	if (limit->burst > 0)
 	{
-		sprintf(buf, "%u", limit->burst);
+		sprintf(buf, "%d", limit->burst);
 		fw3_ipt_rule_addarg(r, limit->invert, "--limit-burst", buf);
 	}
 }
@@ -1089,7 +1090,7 @@  fw3_ipt_rule_time(struct fw3_ipt_rule *r, struct fw3_time *time)
 				if (p > buf)
 					*p++ = ',';
 
-				p += sprintf(p, "%u", i);
+				p += sprintf(p, "%d", i);
 			}
 		}
 
@@ -1105,7 +1106,7 @@  fw3_ipt_rule_time(struct fw3_ipt_rule *r, struct fw3_time *time)
 				if (p > buf)
 					*p++ = ',';
 
-				p += sprintf(p, "%u", i);
+				p += sprintf(p, "%d", i);
 			}
 		}
 
diff --git a/options.c b/options.c
index f686cf0..8b2a8fd 100644
--- a/options.c
+++ b/options.c
@@ -1145,7 +1145,7 @@  fw3_address_to_string(struct fw3_address *address, bool allow_invert, bool as_ci
 	}
 	else
 	{
-		p += sprintf(p, "/%u", fw3_netmask2bitlen(address->family,
+		p += sprintf(p, "/%d", fw3_netmask2bitlen(address->family,
 		                                          &address->mask.v6));
 	}
 
diff --git a/ubus.c b/ubus.c
index 5bb4f5d..1c823f1 100644
--- a/ubus.c
+++ b/ubus.c
@@ -260,10 +260,10 @@  static void fw3_ubus_rules_add(struct blob_buf *b, const char *service,
 	}
 
 	if (instance)
-		snprintf(comment, sizeof(comment), "ubus:%s[%s] %s %d",
+		snprintf(comment, sizeof(comment), "ubus:%s[%s] %s %u",
 				service, instance, type ? type : "rule", n);
 	else
-		snprintf(comment, sizeof(comment), "ubus:%s %s %d",
+		snprintf(comment, sizeof(comment), "ubus:%s %s %u",
 				service, type ? type : "rule", n);
 
 	blobmsg_add_string(b, "name", comment);