| Message ID | 20231115023703.15417-1-heminhong@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | [v2] iproute2: prevent memory leak | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
heminhong <heminhong@kylinos.cn> wrote: > When the return value of rtnl_talk() is less than 0, 'answer' does not > need to release. When the return value of rtnl_talk() is greater than > or equal to 0, 'answer' will be allocated, if subsequent processing fails, > the memory should be free, otherwise it will cause memory leak. I don't understand this patch. Care to elaborate where a memory leak is? rtnl_talk -> __rtnl_talk -> __rtnl_talk_iov 998 static int __rtnl_talk_iov(struct rtnl_handle *rtnl, struct iovec *iov, 999 size_t iovlen, struct nlmsghdr **answer, 1000 bool show_rtnl_err, nl_ext_ack_fn_t errfn) [..] 1102 if (answer) 1103 *answer = (struct nlmsghdr *)buf; 1104 else 1105 free(buf); 1106 return 0; 1107 } 1108 1109 if (answer) { 1110 *answer = (struct nlmsghdr *)buf; 1111 return 0; 1112 } I see no other spots where 'answer' is set, i.e. assignment ONLY on 'return 0'. > Signed-off-by: heminhong <heminhong@kylinos.cn> > --- > ip/link_gre.c | 2 ++ > ip/link_gre6.c | 2 ++ > ip/link_ip6tnl.c | 2 ++ > ip/link_iptnl.c | 2 ++ > ip/link_vti.c | 2 ++ > ip/link_vti6.c | 2 ++ > 6 files changed, 12 insertions(+) > > diff --git a/ip/link_gre.c b/ip/link_gre.c > index 74a5b5e9..b86ec22d 100644 > --- a/ip/link_gre.c > +++ b/ip/link_gre.c > @@ -111,6 +111,8 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv, > > if (rtnl_talk(&rth, &req.n, &answer) < 0) { > get_failed: > + if (answer) > + free(answer); This if() is not needed. free(NULL) is fine. But in this case, 'answer' can contain stack-garbage, as this variable isn't initialised to NULL. Moreover, the placement would need to be ABOVE the label, not below. But, see above, I don't see a 'answer' related memleak.
On Wed, 15 Nov 2023 10:37:03 +0800 heminhong <heminhong@kylinos.cn> wrote: > When the return value of rtnl_talk() is less than 0, 'answer' does not > need to release. When the return value of rtnl_talk() is greater than > or equal to 0, 'answer' will be allocated, if subsequent processing fails, > the memory should be free, otherwise it will cause memory leak. > > Signed-off-by: heminhong <heminhong@kylinos.cn> No null check needed before free(). free() The free() function frees the memory space pointed to by ptr, which must have been returned by a previous call to malloc() or related func‐ tions. Otherwise, or if ptr has already been freed, undefined behavior occurs. If ptr is NULL, no operation is performed.
diff --git a/ip/link_gre.c b/ip/link_gre.c index 74a5b5e9..b86ec22d 100644 --- a/ip/link_gre.c +++ b/ip/link_gre.c @@ -111,6 +111,8 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv, if (rtnl_talk(&rth, &req.n, &answer) < 0) { get_failed: + if (answer) + free(answer); fprintf(stderr, "Failed to get existing tunnel info.\n"); return -1; diff --git a/ip/link_gre6.c b/ip/link_gre6.c index b03bd65a..72ce148a 100644 --- a/ip/link_gre6.c +++ b/ip/link_gre6.c @@ -113,6 +113,8 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv, if (rtnl_talk(&rth, &req.n, &answer) < 0) { get_failed: + if (answer) + free(answer); fprintf(stderr, "Failed to get existing tunnel info.\n"); return -1; diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c index b27d696f..cffa8345 100644 --- a/ip/link_ip6tnl.c +++ b/ip/link_ip6tnl.c @@ -99,6 +99,8 @@ static int ip6tunnel_parse_opt(struct link_util *lu, int argc, char **argv, if (rtnl_talk(&rth, &req.n, &answer) < 0) { get_failed: + if (answer) + free(answer); fprintf(stderr, "Failed to get existing tunnel info.\n"); return -1; diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c index 1315aebe..b4ffed25 100644 --- a/ip/link_iptnl.c +++ b/ip/link_iptnl.c @@ -103,6 +103,8 @@ static int iptunnel_parse_opt(struct link_util *lu, int argc, char **argv, if (rtnl_talk(&rth, &req.n, &answer) < 0) { get_failed: + if (answer) + free(answer); fprintf(stderr, "Failed to get existing tunnel info.\n"); return -1; diff --git a/ip/link_vti.c b/ip/link_vti.c index 50943254..3e4fd95e 100644 --- a/ip/link_vti.c +++ b/ip/link_vti.c @@ -67,6 +67,8 @@ static int vti_parse_opt(struct link_util *lu, int argc, char **argv, if (rtnl_talk(&rth, &req.n, &answer) < 0) { get_failed: + if (answer) + free(answer); fprintf(stderr, "Failed to get existing tunnel info.\n"); return -1; diff --git a/ip/link_vti6.c b/ip/link_vti6.c index 5764221e..e70f5612 100644 --- a/ip/link_vti6.c +++ b/ip/link_vti6.c @@ -69,6 +69,8 @@ static int vti6_parse_opt(struct link_util *lu, int argc, char **argv, if (rtnl_talk(&rth, &req.n, &answer) < 0) { get_failed: + if (answer) + free(answer); fprintf(stderr, "Failed to get existing tunnel info.\n"); return -1;
When the return value of rtnl_talk() is less than 0, 'answer' does not need to release. When the return value of rtnl_talk() is greater than or equal to 0, 'answer' will be allocated, if subsequent processing fails, the memory should be free, otherwise it will cause memory leak. Signed-off-by: heminhong <heminhong@kylinos.cn> --- ip/link_gre.c | 2 ++ ip/link_gre6.c | 2 ++ ip/link_ip6tnl.c | 2 ++ ip/link_iptnl.c | 2 ++ ip/link_vti.c | 2 ++ ip/link_vti6.c | 2 ++ 6 files changed, 12 insertions(+)