[PATCH] iproute2: prevent memory leak

[PATCH] iproute2: prevent memory leak

[heminhong]

The 'rtnl_talk' allocated memory for 'answer',
in the exception handling branch, memory should be free,
otherwise it will cause memory leak.

Signed-off-by: heminhong <heminhong@kylinos.cn>
---
 ip/link_gre.c    | 4 ++++
 ip/link_gre6.c   | 4 ++++
 ip/link_ip6tnl.c | 4 ++++
 ip/link_iptnl.c  | 4 ++++
 ip/link_vti.c    | 4 ++++
 ip/link_vti6.c   | 4 ++++
 6 files changed, 24 insertions(+)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 74a5b5e9..b1c49ace 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -111,6 +111,10 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 
 		if (rtnl_talk(&rth, &req.n, &answer) < 0) {
 get_failed:
+			if (NULL != 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..64302d63 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -113,6 +113,10 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 
 		if (rtnl_talk(&rth, &req.n, &answer) < 0) {
 get_failed:
+			if (NULL != 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..16ed6e0d 100644
--- a/ip/link_ip6tnl.c
+++ b/ip/link_ip6tnl.c
@@ -99,6 +99,10 @@ static int ip6tunnel_parse_opt(struct link_util *lu, int argc, char **argv,
 
 		if (rtnl_talk(&rth, &req.n, &answer) < 0) {
 get_failed:
+			if (NULL != 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..27326382 100644
--- a/ip/link_iptnl.c
+++ b/ip/link_iptnl.c
@@ -103,6 +103,10 @@ static int iptunnel_parse_opt(struct link_util *lu, int argc, char **argv,
 
 		if (rtnl_talk(&rth, &req.n, &answer) < 0) {
 get_failed:
+			if (NULL != 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..92d5c5ad 100644
--- a/ip/link_vti.c
+++ b/ip/link_vti.c
@@ -67,6 +67,10 @@ static int vti_parse_opt(struct link_util *lu, int argc, char **argv,
 
 		if (rtnl_talk(&rth, &req.n, &answer) < 0) {
 get_failed:
+			if (NULL != 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..b50158fe 100644
--- a/ip/link_vti6.c
+++ b/ip/link_vti6.c
@@ -69,6 +69,10 @@ static int vti6_parse_opt(struct link_util *lu, int argc, char **argv,
 
 		if (rtnl_talk(&rth, &req.n, &answer) < 0) {
 get_failed:
+			if (NULL != answer)
+			{
+				free(answer);
+			}
 			fprintf(stderr,
 				"Failed to get existing tunnel info.\n");
 			return -1;
-- 
2.25.1

Re: [PATCH] iproute2: prevent memory leak

[Stephen Hemminger]

On Tue, 14 Nov 2023 17:24:10 +0800
heminhong <heminhong@kylinos.cn> wrote:

> +			if (NULL != answer)
> +			{
> +				free(answer);
> +			}

Four style problems here:
	1. Don't use Windows convention of NULL first
	2. Don't use Windows style bracket indentation
	3. Brackets are not used in kernel style for single statment if()
	4. Since free of NULL is a perfectly valid nop. Just call free() and skip the if.

If you read the existing code, you would see that it uses kernel style.
Any change should follow the conventions of existing code.

[PATCH v2] iproute2: prevent memory leak

[heminhong]

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(+)

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;
-- 
2.25.1

Re: [PATCH v2] iproute2: prevent memory leak

[Florian Westphal]

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.

Re: [PATCH v2] iproute2: prevent memory leak

[Stephen Hemminger]

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.

[PATCH v3] iproute2: prevent memory leak

[heminhong]

When the return value of rtnl_talk() is greater than
or equal to 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    | 1 +
 ip/link_gre6.c   | 1 +
 ip/link_ip6tnl.c | 1 +
 ip/link_iptnl.c  | 1 +
 ip/link_vti.c    | 1 +
 ip/link_vti6.c   | 1 +
 6 files changed, 6 insertions(+)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 74a5b5e9..3d3fcbae 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -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..d74472d2 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -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..8498b726 100644
--- a/ip/link_ip6tnl.c
+++ b/ip/link_ip6tnl.c
@@ -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..2ee4011d 100644
--- a/ip/link_iptnl.c
+++ b/ip/link_iptnl.c
@@ -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..dbbfcb2b 100644
--- a/ip/link_vti.c
+++ b/ip/link_vti.c
@@ -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..096759e6 100644
--- a/ip/link_vti6.c
+++ b/ip/link_vti6.c
@@ -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;
 		}
 
-- 
2.25.1

Re: [PATCH v3] iproute2: prevent memory leak

[Petr Machata]

heminhong <heminhong@kylinos.cn> writes:

> When the return value of rtnl_talk() is greater than
> or equal to 0, 'answer' will be allocated.
> The 'answer' should be free after using,
> otherwise it will cause memory leak.

The patch also frees the memory when rtnl_talk() returns < 0. In that
case the answer has not been initialized. You should probably initialize
the answer to NULL.

> Signed-off-by: heminhong <heminhong@kylinos.cn>
> ---
>  ip/link_gre.c    | 1 +
>  ip/link_gre6.c   | 1 +
>  ip/link_ip6tnl.c | 1 +
>  ip/link_iptnl.c  | 1 +
>  ip/link_vti.c    | 1 +
>  ip/link_vti6.c   | 1 +
>  6 files changed, 6 insertions(+)
>
> diff --git a/ip/link_gre.c b/ip/link_gre.c
> index 74a5b5e9..3d3fcbae 100644
> --- a/ip/link_gre.c
> +++ b/ip/link_gre.c
> @@ -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;
>  		}
>  

The context of this hunk is:

 	struct nlmsghdr *answer;
 
 	... answer untouched here ...
 
 		if (rtnl_talk(&rth, &req.n, &answer) < 0) {
 get_failed:
+			free(answer);
 			fprintf(stderr,
 				"Failed to get existing tunnel info.\n");
 			return -1;
 		}

If rtnl_talk() fails, answer is never initialized, so passing it to free
is invalid.

[PATCH v4] iproute2: prevent memory leak

[heminhong]

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(-)

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;
 		}
 
-- 
2.25.1

Re: [PATCH v4] iproute2: prevent memory leak

[Andrea Claudi]

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>

Re: [PATCH v4] iproute2: prevent memory leak

[Stephen Hemminger]

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

Re: [PATCH v4] iproute2: prevent memory leak

[Andrea Claudi]

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.

Re: [PATCH v4] iproute2: prevent memory leak

[Stephen Hemminger]

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

Re: [PATCH v4] iproute2: prevent memory leak

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html