| Message ID | 20231116031308.16519-1-heminhong@kylinos.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2c3ebb2ae08a634615e56303d784ddb366e47f04 |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | [v4] iproute2: prevent memory leak | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Nov 16, 2023 at 11:13:08AM +0800, heminhong wrote: > When the return value of rtnl_talk() is not less than 0, > 'answer' will be allocated. The 'answer' should be free > after using, otherwise it will cause memory leak. > > Signed-off-by: heminhong <heminhong@kylinos.cn> Reviewed-by: Andrea Claudi <aclaudi@redhat.com>
On Thu, 16 Nov 2023 11:13:08 +0800 heminhong <heminhong@kylinos.cn> wrote: > When the return value of rtnl_talk() is not less than 0, > 'answer' will be allocated. The 'answer' should be free > after using, otherwise it will cause memory leak. > > Signed-off-by: heminhong <heminhong@kylinos.cn> I am skeptical, what is the code path through rtn_talk() that returns non zero, and allocates answer. If so, that should be fixed there. In current code, the returns are: - sendmsg() fails - recvmsg() fails - truncated message The paths that set answer are returning 0
On Thu, Nov 16, 2023 at 03:05:21PM -0800, Stephen Hemminger wrote: > On Thu, 16 Nov 2023 11:13:08 +0800 > heminhong <heminhong@kylinos.cn> wrote: > > > When the return value of rtnl_talk() is not less than 0, > > 'answer' will be allocated. The 'answer' should be free > > after using, otherwise it will cause memory leak. > > > > Signed-off-by: heminhong <heminhong@kylinos.cn> > > I am skeptical, what is the code path through rtn_talk() that > returns non zero, and allocates answer. If so, that should be fixed > there. > > In current code, the returns are: > - sendmsg() fails > - recvmsg() fails > - truncated message > > The paths that set answer are returning 0 IMHO the memory leak is in the same functions this is patching. For example, in ip/link_gre.c:122 we are effectively returning after having answer allocated correctly by rtnl_talk(). The confusion here stems from the fact we are jumping into the error path of rtnl_talk() after rtnl_talk() executed fine.
On Fri, 17 Nov 2023 01:45:51 +0100 Andrea Claudi <aclaudi@redhat.com> wrote: > On Thu, Nov 16, 2023 at 03:05:21PM -0800, Stephen Hemminger wrote: > > On Thu, 16 Nov 2023 11:13:08 +0800 > > heminhong <heminhong@kylinos.cn> wrote: > > > > > When the return value of rtnl_talk() is not less than 0, > > > 'answer' will be allocated. The 'answer' should be free > > > after using, otherwise it will cause memory leak. > > > > > > Signed-off-by: heminhong <heminhong@kylinos.cn> > > > > I am skeptical, what is the code path through rtn_talk() that > > returns non zero, and allocates answer. If so, that should be fixed > > there. > > > > In current code, the returns are: > > - sendmsg() fails > > - recvmsg() fails > > - truncated message > > > > The paths that set answer are returning 0 > > IMHO the memory leak is in the same functions this is patching. > For example, in ip/link_gre.c:122 we are effectively returning after > having answer allocated correctly by rtnl_talk(). > > The confusion here stems from the fact we are jumping into the error > path of rtnl_talk() after rtnl_talk() executed fine. > So looks like a GRE etc bug introduced by the change to parsing. Should add: Fixes: a066cc6623e1 ("gre/gre6: Unify local/remote endpoint address parsing") Cc: serhe.popovych@gmail.com
Hello: This patch was applied to iproute2/iproute2.git (main) by Stephen Hemminger <stephen@networkplumber.org>: On Thu, 16 Nov 2023 11:13:08 +0800 you wrote: > When the return value of rtnl_talk() is not less than 0, > 'answer' will be allocated. The 'answer' should be free > after using, otherwise it will cause memory leak. > > Signed-off-by: heminhong <heminhong@kylinos.cn> > --- > ip/link_gre.c | 3 ++- > ip/link_gre6.c | 3 ++- > ip/link_ip6tnl.c | 3 ++- > ip/link_iptnl.c | 3 ++- > ip/link_vti.c | 3 ++- > ip/link_vti6.c | 3 ++- > 6 files changed, 12 insertions(+), 6 deletions(-) Here is the summary with links: - [v4] iproute2: prevent memory leak https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=2c3ebb2ae08a You are awesome, thank you!
diff --git a/ip/link_gre.c b/ip/link_gre.c index 74a5b5e9..6d71864c 100644 --- a/ip/link_gre.c +++ b/ip/link_gre.c @@ -76,7 +76,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv, .i.ifi_family = preferred_family, .i.ifi_index = ifi->ifi_index, }; - struct nlmsghdr *answer; + struct nlmsghdr *answer = NULL; struct rtattr *tb[IFLA_MAX + 1]; struct rtattr *linkinfo[IFLA_INFO_MAX+1]; struct rtattr *greinfo[IFLA_GRE_MAX + 1]; @@ -113,6 +113,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv, get_failed: fprintf(stderr, "Failed to get existing tunnel info.\n"); + free(answer); return -1; } diff --git a/ip/link_gre6.c b/ip/link_gre6.c index b03bd65a..4d1c6574 100644 --- a/ip/link_gre6.c +++ b/ip/link_gre6.c @@ -79,7 +79,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv, .i.ifi_family = preferred_family, .i.ifi_index = ifi->ifi_index, }; - struct nlmsghdr *answer; + struct nlmsghdr *answer = NULL; struct rtattr *tb[IFLA_MAX + 1]; struct rtattr *linkinfo[IFLA_INFO_MAX+1]; struct rtattr *greinfo[IFLA_GRE_MAX + 1]; @@ -115,6 +115,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv, get_failed: fprintf(stderr, "Failed to get existing tunnel info.\n"); + free(answer); return -1; } diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c index b27d696f..3a30dca9 100644 --- a/ip/link_ip6tnl.c +++ b/ip/link_ip6tnl.c @@ -72,7 +72,7 @@ static int ip6tunnel_parse_opt(struct link_util *lu, int argc, char **argv, .i.ifi_family = preferred_family, .i.ifi_index = ifi->ifi_index, }; - struct nlmsghdr *answer; + struct nlmsghdr *answer = NULL; struct rtattr *tb[IFLA_MAX + 1]; struct rtattr *linkinfo[IFLA_INFO_MAX+1]; struct rtattr *iptuninfo[IFLA_IPTUN_MAX + 1]; @@ -101,6 +101,7 @@ static int ip6tunnel_parse_opt(struct link_util *lu, int argc, char **argv, get_failed: fprintf(stderr, "Failed to get existing tunnel info.\n"); + free(answer); return -1; } diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c index 1315aebe..879202f7 100644 --- a/ip/link_iptnl.c +++ b/ip/link_iptnl.c @@ -73,7 +73,7 @@ static int iptunnel_parse_opt(struct link_util *lu, int argc, char **argv, .i.ifi_family = preferred_family, .i.ifi_index = ifi->ifi_index, }; - struct nlmsghdr *answer; + struct nlmsghdr *answer = NULL; struct rtattr *tb[IFLA_MAX + 1]; struct rtattr *linkinfo[IFLA_INFO_MAX+1]; struct rtattr *iptuninfo[IFLA_IPTUN_MAX + 1]; @@ -105,6 +105,7 @@ static int iptunnel_parse_opt(struct link_util *lu, int argc, char **argv, get_failed: fprintf(stderr, "Failed to get existing tunnel info.\n"); + free(answer); return -1; } diff --git a/ip/link_vti.c b/ip/link_vti.c index 50943254..7a95dc02 100644 --- a/ip/link_vti.c +++ b/ip/link_vti.c @@ -48,7 +48,7 @@ static int vti_parse_opt(struct link_util *lu, int argc, char **argv, .i.ifi_family = preferred_family, .i.ifi_index = ifi->ifi_index, }; - struct nlmsghdr *answer; + struct nlmsghdr *answer = NULL; struct rtattr *tb[IFLA_MAX + 1]; struct rtattr *linkinfo[IFLA_INFO_MAX+1]; struct rtattr *vtiinfo[IFLA_VTI_MAX + 1]; @@ -69,6 +69,7 @@ static int vti_parse_opt(struct link_util *lu, int argc, char **argv, get_failed: fprintf(stderr, "Failed to get existing tunnel info.\n"); + free(answer); return -1; } diff --git a/ip/link_vti6.c b/ip/link_vti6.c index 5764221e..aaf701d3 100644 --- a/ip/link_vti6.c +++ b/ip/link_vti6.c @@ -50,7 +50,7 @@ static int vti6_parse_opt(struct link_util *lu, int argc, char **argv, .i.ifi_family = preferred_family, .i.ifi_index = ifi->ifi_index, }; - struct nlmsghdr *answer; + struct nlmsghdr *answer = NULL; struct rtattr *tb[IFLA_MAX + 1]; struct rtattr *linkinfo[IFLA_INFO_MAX+1]; struct rtattr *vtiinfo[IFLA_VTI_MAX + 1]; @@ -71,6 +71,7 @@ static int vti6_parse_opt(struct link_util *lu, int argc, char **argv, get_failed: fprintf(stderr, "Failed to get existing tunnel info.\n"); + free(answer); return -1; }
When the return value of rtnl_talk() is not less than 0, 'answer' will be allocated. The 'answer' should be free after using, otherwise it will cause memory leak. Signed-off-by: heminhong <heminhong@kylinos.cn> --- ip/link_gre.c | 3 ++- ip/link_gre6.c | 3 ++- ip/link_ip6tnl.c | 3 ++- ip/link_iptnl.c | 3 ++- ip/link_vti.c | 3 ++- ip/link_vti6.c | 3 ++- 6 files changed, 12 insertions(+), 6 deletions(-)