Re: [PATCH iproute2] ss: Fix allocation of cong control alg name
On 06/25/2015 05:31 AM, Stephen Hemminger wrote:
On Fri, 29 May 2015 18:48:42 +0200
Daniel Borkmann dan...@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. ;)
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH iproute2] ss: Fix allocation of cong control alg name
On Fri, 29 May 2015 18:48:42 +0200
Daniel Borkmann dan...@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.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH iproute2] ss: Fix allocation of cong control alg name
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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 vadi...@gmail.com
Use strdup instead of malloc, and get rid of bad strcpy.
Signed-off-by: Vadim Kochan vadi...@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.mosqu...@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)) {
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH iproute2] ss: Fix allocation of cong control alg name
On Fri, 2015-05-29 at 13:30 +0300, Vadim Kochan wrote:
From: Vadim Kochan vadi...@gmail.com
Use strdup instead of malloc, and get rid of bad strcpy.
Signed-off-by: Vadim Kochan vadi...@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;
+ charcong_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);
}
}
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH iproute2] ss: Fix allocation of cong control alg name
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 vadi...@gmail.com
Use strdup instead of malloc, and get rid of bad strcpy.
Signed-off-by: Vadim Kochan vadi...@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;
+ charcong_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 ?
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH iproute2] ss: Fix allocation of cong control alg name
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:dan...@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 vadi...@gmail.com
Use strdup instead of malloc, and get rid of bad strcpy.
Signed-off-by: Vadim Kochan vadi...@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.mosqu...@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)) {
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH iproute2] ss: Fix allocation of cong control alg name
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. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH iproute2] ss: Fix allocation of cong control alg name
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);
}
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
