| Message ID | 20150715151204.GB28525@angus-think.wlc.globallogic.com |
|---|---|
| State | RFC, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
> On Jul 15, 2015, at 8:12 AM, Vadim Kochan <vadim4j@gmail.com> wrote: > Would you please check this fix ? > > diff --git a/misc/ss.c b/misc/ss.c > index 03f92fa..3a826e4 100644 > --- a/misc/ss.c > +++ b/misc/ss.c > @@ -683,8 +683,8 @@ static inline void sock_addr_set_str(inet_prefix *prefix, char **ptr) > > static inline char *sock_addr_get_str(const inet_prefix *prefix) > { > - char *tmp ; > - memcpy(&tmp, prefix->data, sizeof(char *)); > + char *tmp; > + memcpy(&tmp, &prefix->data[0], sizeof(char *)); > return tmp; > } That surely is not a fix! The destination of the memcpy is the address of an uninitialized stack variable! Both versions are equally bad. -- Mark Rustad, Networking Division, Intel Corporation
> On Jul 15, 2015, at 9:49 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: > >> On Jul 15, 2015, at 8:12 AM, Vadim Kochan <vadim4j@gmail.com> wrote: >> Would you please check this fix ? >> >> diff --git a/misc/ss.c b/misc/ss.c >> index 03f92fa..3a826e4 100644 >> --- a/misc/ss.c >> +++ b/misc/ss.c >> @@ -683,8 +683,8 @@ static inline void sock_addr_set_str(inet_prefix *prefix, char **ptr) >> >> static inline char *sock_addr_get_str(const inet_prefix *prefix) >> { >> - char *tmp ; >> - memcpy(&tmp, prefix->data, sizeof(char *)); >> + char *tmp; >> + memcpy(&tmp, &prefix->data[0], sizeof(char *)); >> return tmp; >> } > > That surely is not a fix! The destination of the memcpy is the address of an uninitialized stack variable! Both versions are equally bad. I probably over-reacted, but using memcpy to access a pointer in this way is just ugly. For one thing, it circumvents any sanity-checking that the compiler can do. And changing the prefix->data to &prefix->data[0] should be exactly the same thing and therefore should not fix anything. Anyway, never mind that. Looking at more of the code, it looks to me like the the string pointer in data can sometimes point to a literal string instead of allocated memory when proc is in use. Free would not be happy with that. Look at the use of variable peer in function unix_stats_print. -- Mark Rustad, Networking Division, Intel Corporation
On Wed, Jul 15, 2015 at 06:52:49PM +0000, Rustad, Mark D wrote: > > On Jul 15, 2015, at 9:49 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: > > > >> On Jul 15, 2015, at 8:12 AM, Vadim Kochan <vadim4j@gmail.com> wrote: > >> Would you please check this fix ? > >> > >> diff --git a/misc/ss.c b/misc/ss.c > >> index 03f92fa..3a826e4 100644 > >> --- a/misc/ss.c > >> +++ b/misc/ss.c > >> @@ -683,8 +683,8 @@ static inline void sock_addr_set_str(inet_prefix *prefix, char **ptr) > >> > >> static inline char *sock_addr_get_str(const inet_prefix *prefix) > >> { > >> - char *tmp ; > >> - memcpy(&tmp, prefix->data, sizeof(char *)); > >> + char *tmp; > >> + memcpy(&tmp, &prefix->data[0], sizeof(char *)); > >> return tmp; > >> } > > > > That surely is not a fix! The destination of the memcpy is the address of an uninitialized stack variable! Both versions are equally bad. > > I probably over-reacted, but using memcpy to access a pointer in this way is just ugly. For one thing, it circumvents any sanity-checking that the compiler can do. And changing the prefix->data to &prefix->data[0] should be exactly the same thing and therefore should not fix anything. Anyway, never mind that. > > Looking at more of the code, it looks to me like the the string pointer in data can sometimes point to a literal string instead of allocated memory when proc is in use. Free would not be happy with that. Look at the use of variable peer in function unix_stats_print. > Yes that right, I am already looking on this ... > -- > Mark Rustad, Networking Division, Intel Corporation > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 15, 2015 at 09:57:51PM +0300, Vadim Kochan wrote: > On Wed, Jul 15, 2015 at 06:52:49PM +0000, Rustad, Mark D wrote: > > > On Jul 15, 2015, at 9:49 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: > > > > > >> On Jul 15, 2015, at 8:12 AM, Vadim Kochan <vadim4j@gmail.com> wrote: > > >> Would you please check this fix ? > > >> > > >> diff --git a/misc/ss.c b/misc/ss.c > > >> index 03f92fa..3a826e4 100644 > > >> --- a/misc/ss.c > > >> +++ b/misc/ss.c > > >> @@ -683,8 +683,8 @@ static inline void sock_addr_set_str(inet_prefix *prefix, char **ptr) > > >> > > >> static inline char *sock_addr_get_str(const inet_prefix *prefix) > > >> { > > >> - char *tmp ; > > >> - memcpy(&tmp, prefix->data, sizeof(char *)); > > >> + char *tmp; > > >> + memcpy(&tmp, &prefix->data[0], sizeof(char *)); > > >> return tmp; > > >> } > > > > > > That surely is not a fix! The destination of the memcpy is the address of an uninitialized stack variable! Both versions are equally bad. > > > > I probably over-reacted, but using memcpy to access a pointer in this way is just ugly. For one thing, it circumvents any sanity-checking that the compiler can do. And changing the prefix->data to &prefix->data[0] should be exactly the same thing and therefore should not fix anything. Anyway, never mind that. > > > > Looking at more of the code, it looks to me like the the string pointer in data can sometimes point to a literal string instead of allocated memory when proc is in use. Free would not be happy with that. Look at the use of variable peer in function unix_stats_print. > > > Yes that right, I am already looking on this ... > > -- > > Mark Rustad, Networking Division, Intel Corporation > > I did partially revert of the buggy commit and it does not crash, but I will do more testing, and after will send the patch and will try to prepare some test scripts for ss. The crash appears only if to dump processes info from /proc, which might be caused that netlink stats returned error, probably by wrong request (not supported attribute or flag ?). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, 16. Juli 2015, 01:22:38 schrieb Vadim Kochan: > On Wed, Jul 15, 2015 at 09:57:51PM +0300, Vadim Kochan wrote: > > On Wed, Jul 15, 2015 at 06:52:49PM +0000, Rustad, Mark D wrote: > > > > On Jul 15, 2015, at 9:49 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: > > > >> On Jul 15, 2015, at 8:12 AM, Vadim Kochan <vadim4j@gmail.com> wrote: > > > >> Would you please check this fix ? > > > >> > > > >> diff --git a/misc/ss.c b/misc/ss.c > > > >> index 03f92fa..3a826e4 100644 > > > >> --- a/misc/ss.c > > > >> +++ b/misc/ss.c > > > >> @@ -683,8 +683,8 @@ static inline void sock_addr_set_str(inet_prefix > > > >> *prefix, char **ptr) > > > >> > > > >> static inline char *sock_addr_get_str(const inet_prefix *prefix) > > > >> { > > > >> - char *tmp ; > > > >> - memcpy(&tmp, prefix->data, sizeof(char *)); > > > >> + char *tmp; > > > >> + memcpy(&tmp, &prefix->data[0], sizeof(char *)); > > > >> > > > >> return tmp; > > > >> > > > >> } > > > > > > > > That surely is not a fix! The destination of the memcpy is the address > > > > of an uninitialized stack variable! Both versions are equally bad.> > > > > I probably over-reacted, but using memcpy to access a pointer in this > > > way is just ugly. For one thing, it circumvents any sanity-checking > > > that the compiler can do. And changing the prefix->data to > > > &prefix->data[0] should be exactly the same thing and therefore should > > > not fix anything. Anyway, never mind that. > > > > > > Looking at more of the code, it looks to me like the the string pointer > > > in data can sometimes point to a literal string instead of allocated > > > memory when proc is in use. Free would not be happy with that. Look at > > > the use of variable peer in function unix_stats_print.> > > Yes that right, I am already looking on this ... > > > > > -- > > > Mark Rustad, Networking Division, Intel Corporation > > I did partially revert of the buggy commit and it does not crash, but I will > do more testing, and after will send the patch and will try to prepare some > test scripts for ss. > > The crash appears only if to dump processes info from /proc, which might > be caused that netlink stats returned error, probably by wrong request > (not supported attribute or flag ?). the reason it uses proc for me is my self compiled (and trimmed) kernel which disabled all *_diag modules which seem to be required by ss. I didn't know this. On the other hand, I managed to find a bug this way :-) Marc
diff --git a/misc/ss.c b/misc/ss.c index 03f92fa..3a826e4 100644 --- a/misc/ss.c +++ b/misc/ss.c @@ -683,8 +683,8 @@ static inline void sock_addr_set_str(inet_prefix *prefix, char **ptr) static inline char *sock_addr_get_str(const inet_prefix *prefix) { - char *tmp ; - memcpy(&tmp, prefix->data, sizeof(char *)); + char *tmp; + memcpy(&tmp, &prefix->data[0], sizeof(char *)); return tmp; }