[PATCH iproute2] ss: Fix allocation of cong control alg name

[PATCH iproute2] ss: Fix allocation of cong control alg name

[Vadim Kochan]

From: Vadim Kochan <vadim4j@gmail.com>

Use strdup instead of malloc, and get rid of bad strcpy.

Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
 misc/ss.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index 347e3a1..a719466 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -1908,8 +1908,7 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
 
 		if (tb[INET_DIAG_CONG]) {
 			const char *cong_attr = rta_getattr_str(tb[INET_DIAG_CONG]);
-			s.cong_alg = malloc(strlen(cong_attr + 1));
-			strcpy(s.cong_alg, cong_attr);
+			s.cong_alg = strdup(cong_attr);
 		}
 
 		if (TCPI_HAS_OPT(info, TCPI_OPT_WSCALE)) {
-- 
2.3.1

Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Eric Dumazet]

On Fri, 2015-05-29 at 13:30 +0300, Vadim Kochan wrote:
> From: Vadim Kochan <vadim4j@gmail.com>
> 
> Use strdup instead of malloc, and get rid of bad strcpy.
> 
> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
> ---
>  misc/ss.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index 347e3a1..a719466 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -1908,8 +1908,7 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
>  
>  		if (tb[INET_DIAG_CONG]) {
>  			const char *cong_attr = rta_getattr_str(tb[INET_DIAG_CONG]);
> -			s.cong_alg = malloc(strlen(cong_attr + 1));
> -			strcpy(s.cong_alg, cong_attr);
> +			s.cong_alg = strdup(cong_attr);
>  		}
>  
>  		if (TCPI_HAS_OPT(info, TCPI_OPT_WSCALE)) {

I doubt TCP_CA_NAME_MAX will ever change in the kernel : 16 bytes.

Its typically "cubic" and less than 8 bytes.

Using 8 bytes to point to a malloc(8) is a waste.

Please remove the memory allocation, or store the pointer, since
tcp_show_info() does the malloc()/free() before return.

diff --git a/misc/ss.c b/misc/ss.c
index 347e3a1..9fe229f 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -755,7 +755,7 @@ struct tcpstat
 	int		    timer;
 	int		    timeout;
 	int		    probes;
-	char		    *cong_alg;
+	char		    cong_alg[16];
 	double		    rto, ato, rtt, rttvar;
 	int		    qack, cwnd, ssthresh, backoff;
 	double		    send_bps;
@@ -1664,7 +1664,7 @@ static void tcp_stats_print(struct tcpstat *s)
 		printf(" ecnseen");
 	if (s->has_fastopen_opt)
 		printf(" fastopen");
-	if (s->cong_alg)
+	if (s->cong_alg[0])
 		printf(" %s", s->cong_alg);
 	if (s->has_wscale_opt)
 		printf(" wscale:%d,%d", s->snd_wscale, s->rcv_wscale);
@@ -1906,11 +1906,10 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
 			s.has_fastopen_opt = TCPI_HAS_OPT(info, TCPI_OPT_SYN_DATA);
 		}
 
-		if (tb[INET_DIAG_CONG]) {
-			const char *cong_attr = rta_getattr_str(tb[INET_DIAG_CONG]);
-			s.cong_alg = malloc(strlen(cong_attr + 1));
-			strcpy(s.cong_alg, cong_attr);
-		}
+		if (tb[INET_DIAG_CONG])
+			strncpy(s.cong_alg,
+				rta_getattr_str(tb[INET_DIAG_CONG]),
+				sizeof(s.cong_alg) - 1);
 
 		if (TCPI_HAS_OPT(info, TCPI_OPT_WSCALE)) {
 			s.has_wscale_opt  = true;
@@ -1984,8 +1983,6 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
 		tcp_stats_print(&s);
 		if (s.dctcp)
 			free(s.dctcp);
-		if (s.cong_alg)
-			free(s.cong_alg);
 	}
 }
 

Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Daniel Borkmann]

On 05/29/2015 01:04 PM, Eric Dumazet wrote:
...
> I doubt TCP_CA_NAME_MAX will ever change in the kernel : 16 bytes.
>
> Its typically "cubic" and less than 8 bytes.
>
> Using 8 bytes to point to a malloc(8) is a waste.
>
> Please remove the memory allocation, or store the pointer, since
> tcp_show_info() does the malloc()/free() before return.

+1, much better

Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Vadim Kochan]

On Fri, May 29, 2015 at 04:04:05AM -0700, Eric Dumazet wrote:
> On Fri, 2015-05-29 at 13:30 +0300, Vadim Kochan wrote:
> > From: Vadim Kochan <vadim4j@gmail.com>
> > 
> > Use strdup instead of malloc, and get rid of bad strcpy.
> > 
> > Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
> > ---
> >  misc/ss.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/misc/ss.c b/misc/ss.c
> > index 347e3a1..a719466 100644
> > --- a/misc/ss.c
> > +++ b/misc/ss.c
> > @@ -1908,8 +1908,7 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
> >  
> >  		if (tb[INET_DIAG_CONG]) {
> >  			const char *cong_attr = rta_getattr_str(tb[INET_DIAG_CONG]);
> > -			s.cong_alg = malloc(strlen(cong_attr + 1));
> > -			strcpy(s.cong_alg, cong_attr);
> > +			s.cong_alg = strdup(cong_attr);
> >  		}
> >  
> >  		if (TCPI_HAS_OPT(info, TCPI_OPT_WSCALE)) {
> 
> I doubt TCP_CA_NAME_MAX will ever change in the kernel : 16 bytes.
> 
> Its typically "cubic" and less than 8 bytes.
> 
> Using 8 bytes to point to a malloc(8) is a waste.
> 
> Please remove the memory allocation, or store the pointer, since
> tcp_show_info() does the malloc()/free() before return.
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index 347e3a1..9fe229f 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -755,7 +755,7 @@ struct tcpstat
>  	int		    timer;
>  	int		    timeout;
>  	int		    probes;
> -	char		    *cong_alg;
> +	char		    cong_alg[16];
>  	double		    rto, ato, rtt, rttvar;
>  	int		    qack, cwnd, ssthresh, backoff;
>  	double		    send_bps;
> @@ -1664,7 +1664,7 @@ static void tcp_stats_print(struct tcpstat *s)
>  		printf(" ecnseen");
>  	if (s->has_fastopen_opt)
>  		printf(" fastopen");
> -	if (s->cong_alg)
> +	if (s->cong_alg[0])
>  		printf(" %s", s->cong_alg);
>  	if (s->has_wscale_opt)
>  		printf(" wscale:%d,%d", s->snd_wscale, s->rcv_wscale);
> @@ -1906,11 +1906,10 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
>  			s.has_fastopen_opt = TCPI_HAS_OPT(info, TCPI_OPT_SYN_DATA);
>  		}
>  
> -		if (tb[INET_DIAG_CONG]) {
> -			const char *cong_attr = rta_getattr_str(tb[INET_DIAG_CONG]);
> -			s.cong_alg = malloc(strlen(cong_attr + 1));
> -			strcpy(s.cong_alg, cong_attr);
> -		}
> +		if (tb[INET_DIAG_CONG])
> +			strncpy(s.cong_alg,
> +				rta_getattr_str(tb[INET_DIAG_CONG]),
> +				sizeof(s.cong_alg) - 1);
>  
>  		if (TCPI_HAS_OPT(info, TCPI_OPT_WSCALE)) {
>  			s.has_wscale_opt  = true;
> @@ -1984,8 +1983,6 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
>  		tcp_stats_print(&s);
>  		if (s.dctcp)
>  			free(s.dctcp);
> -		if (s.cong_alg)
> -			free(s.cong_alg);
>  	}
>  }
>  
> 
> 

Thanks!

Should I put you in "From" tag or in "Signed-off-by" ?
Or your diff might be used from this email thread ?

Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Eric Dumazet]

On Fri, 2015-05-29 at 15:53 +0300, Vadim Kochan wrote:

> Thanks!
> 
> Should I put you in "From" tag or in "Signed-off-by" ?
> Or your diff might be used from this email thread ?

Don't worry, just submit the patch officially on your own ;)

Thanks.

Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Daniel Borkmann]

Hi Vadim,

On 05/29/2015 12:30 PM, Vadim Kochan wrote:
> From: Vadim Kochan <vadim4j@gmail.com>
>
> Use strdup instead of malloc, and get rid of bad strcpy.
>
> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>

Please also Cc the reporter (done here), and add a:

Fixes: 8250bc9ff4e5 ("ss: Unify inet sockets output")
Reported-by: Jose R. Guzman Mosqueda <jose.r.guzman.mosqueda@intel.com>

Fixes tag is _very useful_ for distros to easily identify if additional
follow-up commits would be needed when backporting the original change.
Then, this can be easily identified when going through the git log.

> ---
>   misc/ss.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/misc/ss.c b/misc/ss.c
> index 347e3a1..a719466 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -1908,8 +1908,7 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
>
>   		if (tb[INET_DIAG_CONG]) {
>   			const char *cong_attr = rta_getattr_str(tb[INET_DIAG_CONG]);
> -			s.cong_alg = malloc(strlen(cong_attr + 1));
> -			strcpy(s.cong_alg, cong_attr);
> +			s.cong_alg = strdup(cong_attr);

strdup(3) can still return NULL.

>   		}
>
>   		if (TCPI_HAS_OPT(info, TCPI_OPT_WSCALE)) {
>

RE: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Guzman Mosqueda, Jose R]

Hi Daniel and Vadim

Thanks for your prompt response and for the patch.

Also, what about the other one? Do you think it is an issue or not?

" File: tc/tc_util.c
Function: void print_rate(char *buf, int len, __u64 rate)
Line: ~264

In the case that user inputs a high value for rate, the "for" loop will exit in the condition meaning that variable "i" get the value of 5 which will be an invalid index for the "units" array due to that array has only 5 elements."

I know a very high value is invalid but in the case that it comes directly from user, it could cause and issue, what do you think?

Thanks,
-José R.




-----Original Message-----
From: Daniel Borkmann [mailto:daniel@iogearbox.net] 
Sent: Friday, May 29, 2015 6:10 AM
To: Vadim Kochan
Cc: netdev@vger.kernel.org; Guzman Mosqueda, Jose R
Subject: Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

Hi Vadim,

On 05/29/2015 12:30 PM, Vadim Kochan wrote:
> From: Vadim Kochan <vadim4j@gmail.com>
>
> Use strdup instead of malloc, and get rid of bad strcpy.
>
> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>

Please also Cc the reporter (done here), and add a:

Fixes: 8250bc9ff4e5 ("ss: Unify inet sockets output")
Reported-by: Jose R. Guzman Mosqueda <jose.r.guzman.mosqueda@intel.com>

Fixes tag is _very useful_ for distros to easily identify if additional follow-up commits would be needed when backporting the original change.
Then, this can be easily identified when going through the git log.

> ---
>   misc/ss.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/misc/ss.c b/misc/ss.c
> index 347e3a1..a719466 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -1908,8 +1908,7 @@ static void tcp_show_info(const struct nlmsghdr 
> *nlh, struct inet_diag_msg *r,
>
>   		if (tb[INET_DIAG_CONG]) {
>   			const char *cong_attr = rta_getattr_str(tb[INET_DIAG_CONG]);
> -			s.cong_alg = malloc(strlen(cong_attr + 1));
> -			strcpy(s.cong_alg, cong_attr);
> +			s.cong_alg = strdup(cong_attr);

strdup(3) can still return NULL.

>   		}
>
>   		if (TCPI_HAS_OPT(info, TCPI_OPT_WSCALE)) {
>

Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Daniel Borkmann]

On 05/29/2015 06:17 PM, Guzman Mosqueda, Jose R wrote:
> Hi Daniel and Vadim
>
> Thanks for your prompt response and for the patch.
>
> Also, what about the other one? Do you think it is an issue or not?
>
> " File: tc/tc_util.c
> Function: void print_rate(char *buf, int len, __u64 rate)
> Line: ~264
>
> In the case that user inputs a high value for rate, the "for" loop will exit in the condition meaning that variable "i" get the value of 5 which will be an invalid index for the "units" array due to that array has only 5 elements."
>
> I know a very high value is invalid but in the case that it comes directly from user, it could cause and issue, what do you think?

Hm, this prints just the netlink dump from kernel side, but perhaps
we should just change it ...

diff --git a/tc/tc_util.c b/tc/tc_util.c
index dc2b70f..aa6de24 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -250,18 +250,19 @@ void print_rate(char *buf, int len, __u64 rate)
  	extern int use_iec;
  	unsigned long kilo = use_iec ? 1024 : 1000;
  	const char *str = use_iec ? "i" : "";
-	int i = 0;
  	static char *units[5] = {"", "K", "M", "G", "T"};
+	int i;

  	rate <<= 3; /* bytes/sec -> bits/sec */

-	for (i = 0; i < ARRAY_SIZE(units); i++)  {
+	for (i = 0; i < ARRAY_SIZE(units) - 1; i++)  {
  		if (rate < kilo)
  			break;
  		if (((rate % kilo) != 0) && rate < 1000*kilo)
  			break;
  		rate /= kilo;
  	}
+
  	snprintf(buf, len, "%.0f%s%sbit", (double)rate, units[i], str);
  }

Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Stephen Hemminger]

On Fri, 29 May 2015 18:48:42 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 05/29/2015 06:17 PM, Guzman Mosqueda, Jose R wrote:
> > Hi Daniel and Vadim
> >
> > Thanks for your prompt response and for the patch.
> >
> > Also, what about the other one? Do you think it is an issue or not?
> >
> > " File: tc/tc_util.c
> > Function: void print_rate(char *buf, int len, __u64 rate)
> > Line: ~264
> >
> > In the case that user inputs a high value for rate, the "for" loop will exit in the condition meaning that variable "i" get the value of 5 which will be an invalid index for the "units" array due to that array has only 5 elements."
> >
> > I know a very high value is invalid but in the case that it comes directly from user, it could cause and issue, what do you think?
> 
> Hm, this prints just the netlink dump from kernel side, but perhaps
> we should just change it ...
> 
> diff --git a/tc/tc_util.c b/tc/tc_util.c
> index dc2b70f..aa6de24 100644
> --- a/tc/tc_util.c
> +++ b/tc/tc_util.c
> @@ -250,18 +250,19 @@ void print_rate(char *buf, int len, __u64 rate)
>   	extern int use_iec;
>   	unsigned long kilo = use_iec ? 1024 : 1000;
>   	const char *str = use_iec ? "i" : "";
> -	int i = 0;
>   	static char *units[5] = {"", "K", "M", "G", "T"};
> +	int i;
> 
>   	rate <<= 3; /* bytes/sec -> bits/sec */
> 
> -	for (i = 0; i < ARRAY_SIZE(units); i++)  {
> +	for (i = 0; i < ARRAY_SIZE(units) - 1; i++)  {
>   		if (rate < kilo)
>   			break;
>   		if (((rate % kilo) != 0) && rate < 1000*kilo)
>   			break;
>   		rate /= kilo;
>   	}
> +
>   	snprintf(buf, len, "%.0f%s%sbit", (double)rate, units[i], str);
>   }

I don't know what thread you meant to hijack for this, but it wasn't the
one about ss: cong name.

Re: [PATCH iproute2] ss: Fix allocation of cong control alg name

[Daniel Borkmann]

On 06/25/2015 05:31 AM, Stephen Hemminger wrote:
> On Fri, 29 May 2015 18:48:42 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 05/29/2015 06:17 PM, Guzman Mosqueda, Jose R wrote:
>>> Hi Daniel and Vadim
>>>
>>> Thanks for your prompt response and for the patch.
>>>
>>> Also, what about the other one? Do you think it is an issue or not?
>>>
>>> " File: tc/tc_util.c
>>> Function: void print_rate(char *buf, int len, __u64 rate)
>>> Line: ~264
>>>
>>> In the case that user inputs a high value for rate, the "for" loop will exit in the condition meaning that variable "i" get the value of 5 which will be an invalid index for the "units" array due to that array has only 5 elements."
>>>
>>> I know a very high value is invalid but in the case that it comes directly from user, it could cause and issue, what do you think?
>>
>> Hm, this prints just the netlink dump from kernel side, but perhaps
>> we should just change it ...
>>
>> diff --git a/tc/tc_util.c b/tc/tc_util.c
>> index dc2b70f..aa6de24 100644
>> --- a/tc/tc_util.c
>> +++ b/tc/tc_util.c
>> @@ -250,18 +250,19 @@ void print_rate(char *buf, int len, __u64 rate)
>>    	extern int use_iec;
>>    	unsigned long kilo = use_iec ? 1024 : 1000;
>>    	const char *str = use_iec ? "i" : "";
>> -	int i = 0;
>>    	static char *units[5] = {"", "K", "M", "G", "T"};
>> +	int i;
>>
>>    	rate <<= 3; /* bytes/sec -> bits/sec */
>>
>> -	for (i = 0; i < ARRAY_SIZE(units); i++)  {
>> +	for (i = 0; i < ARRAY_SIZE(units) - 1; i++)  {
>>    		if (rate < kilo)
>>    			break;
>>    		if (((rate % kilo) != 0) && rate < 1000*kilo)
>>    			break;
>>    		rate /= kilo;
>>    	}
>> +
>>    	snprintf(buf, len, "%.0f%s%sbit", (double)rate, units[i], str);
>>    }
>
> I don't know what thread you meant to hijack for this, but it wasn't the
> one about ss: cong name.

Jose did top-post on the first reported issue asking about the 2nd one, I
think that's how we ended up here. ;)