* [PATCH v1 0/2] fix memory leaks reported by GCC -fanalyzer
@ 2023-02-24 13:36 Edwin Török
2023-02-24 13:36 ` [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak Edwin Török
2023-02-24 13:36 ` [PATCH v1 2/2] backup_ptes: fix leak on realloc failure Edwin Török
0 siblings, 2 replies; 9+ messages in thread
From: Edwin Török @ 2023-02-24 13:36 UTC (permalink / raw
To: xen-devel; +Cc: Edwin Török, Wei Liu, Anthony PERARD, Juergen Gross
From: Edwin Török <edwin.torok@cloud.com>
Using GCC 12.2.1 with -fanalyzer it has shown some memory leaks:
This is how I enabled -fanalyzer (adding it to CFLAGS for toplevel
configure didn't seem to work):
```
CFLAGS += $(call cc-option,$(CC),-fanalyzer)
```
Note that there are more errors shown than fixed here, but they seem to
be false positives (which is why this flag cannot, yet, be enabled by
default).
Edwin Török (2):
xc_core_arch_map_p2m_tree_rw: fix memory leak
backup_ptes: fix leak on realloc failure
tools/libs/guest/xg_core_x86.c | 2 ++
tools/libs/guest/xg_offline_page.c | 7 +++++--
2 files changed, 7 insertions(+), 2 deletions(-)
--
2.39.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
2023-02-24 13:36 [PATCH v1 0/2] fix memory leaks reported by GCC -fanalyzer Edwin Török
@ 2023-02-24 13:36 ` Edwin Török
2023-02-24 14:56 ` Andrew Cooper
2023-02-27 15:00 ` [PATCH] libs/guest: Fix resource leaks in xc_core_arch_map_p2m_tree_rw() Andrew Cooper
2023-02-24 13:36 ` [PATCH v1 2/2] backup_ptes: fix leak on realloc failure Edwin Török
1 sibling, 2 replies; 9+ messages in thread
From: Edwin Török @ 2023-02-24 13:36 UTC (permalink / raw
To: xen-devel; +Cc: Edwin Török, Wei Liu, Anthony PERARD, Juergen Gross
From: Edwin Török <edwin.torok@cloud.com>
Prior to bd7a29c3d0 'out' would've always been executed and memory
freed, but that commit changed it such that it returns early and leaks.
Found using gcc 12.2.1 `-fanalyzer`:
```
xg_core_x86.c: In function ‘xc_core_arch_map_p2m_tree_rw’:
xg_core_x86.c:300:5: error: leak of ‘p2m_frame_list_list’ [CWE-401] [-Werror=analyzer-malloc-leak]
300 | return p2m_frame_list;
| ^~~~~~
‘xc_core_arch_map_p2m_writable’: events 1-2
|
| 378 | xc_core_arch_map_p2m_writable(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (1) entry to ‘xc_core_arch_map_p2m_writable’
|......
| 381 | return xc_core_arch_map_p2m_rw(xch, dinfo, info, live_shinfo, live_p2m, 1);
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (2) calling ‘xc_core_arch_map_p2m_rw’ from ‘xc_core_arch_map_p2m_writable’
|
+--> ‘xc_core_arch_map_p2m_rw’: events 3-10
|
| 319 | xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
| | ^~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (3) entry to ‘xc_core_arch_map_p2m_rw’
|......
| 328 | if ( xc_domain_nr_gpfns(xch, info->domid, &dinfo->p2m_size) < 0 )
| | ~
| | |
| | (4) following ‘false’ branch...
|......
| 334 | if ( dinfo->p2m_size < info->nr_pages )
| | ~~ ~
| | | |
| | | (6) following ‘false’ branch...
| | (5) ...to here
|......
| 340 | p2m_cr3 = GET_FIELD(live_shinfo, arch.p2m_cr3, dinfo->guest_width);
| | ~~~~~~~
| | |
| | (7) ...to here
| 341 |
| 342 | p2m_frame_list = p2m_cr3 ? xc_core_arch_map_p2m_list_rw(xch, dinfo, dom, live_shinfo, p2m_cr3)
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| 343 | : xc_core_arch_map_p2m_tree_rw(xch, dinfo, dom, live_shinfo);
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | | |
| | | (9) ...to here
| | | (10) calling ‘xc_core_arch_map_p2m_tree_rw’ from ‘xc_core_arch_map_p2m_rw’
| | (8) following ‘false’ branch...
|
+--> ‘xc_core_arch_map_p2m_tree_rw’: events 11-24
|
| 228 | xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinfo,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (11) entry to ‘xc_core_arch_map_p2m_tree_rw’
|......
| 245 | if ( !live_p2m_frame_list_list )
| | ~
| | |
| | (12) following ‘false’ branch (when ‘live_p2m_frame_list_list’ is non-NULL)...
|......
| 252 | if ( !(p2m_frame_list_list = malloc(PAGE_SIZE)) )
| | ~~ ~ ~~~~~~~~~~~~~~~~~
| | | | |
| | | | (14) allocated here
| | | (15) assuming ‘p2m_frame_list_list’ is non-NULL
| | | (16) following ‘false’ branch (when ‘p2m_frame_list_list’ is non-NULL)...
| | (13) ...to here
|......
| 257 | memcpy(p2m_frame_list_list, live_p2m_frame_list_list, PAGE_SIZE);
| | ~~~~~~
| | |
| | (17) ...to here
|......
| 266 | else if ( dinfo->guest_width < sizeof(unsigned long) )
| | ~
| | |
| | (18) following ‘false’ branch...
|......
| 270 | live_p2m_frame_list =
| | ~~~~~~~~~~~~~~~~~~~
| | |
| | (19) ...to here
|......
| 275 | if ( !live_p2m_frame_list )
| | ~
| | |
| | (20) following ‘false’ branch (when ‘live_p2m_frame_list’ is non-NULL)...
|......
| 282 | if ( !(p2m_frame_list = malloc(P2M_TOOLS_FL_SIZE)) )
| | ~~ ~
| | | |
| | | (22) following ‘false’ branch (when ‘p2m_frame_list’ is non-NULL)...
| | (21) ...to here
|......
| 287 | memset(p2m_frame_list, 0, P2M_TOOLS_FL_SIZE);
| | ~~~~~~
| | |
| | (23) ...to here
|......
| 300 | return p2m_frame_list;
| | ~~~~~~
| | |
| | (24) ‘p2m_frame_list_list’ leaks here; was allocated at (14)
|
```
Fixes: bd7a29c3d0 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
---
tools/libs/guest/xg_core_x86.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
index 61106b98b8..69929879d7 100644
--- a/tools/libs/guest/xg_core_x86.c
+++ b/tools/libs/guest/xg_core_x86.c
@@ -297,6 +297,8 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinf
dinfo->p2m_frames = P2M_FL_ENTRIES;
+ free(p2m_frame_list_list);
+
return p2m_frame_list;
out:
--
2.39.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v1 2/2] backup_ptes: fix leak on realloc failure
2023-02-24 13:36 [PATCH v1 0/2] fix memory leaks reported by GCC -fanalyzer Edwin Török
2023-02-24 13:36 ` [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak Edwin Török
@ 2023-02-24 13:36 ` Edwin Török
2023-02-24 15:00 ` Andrew Cooper
1 sibling, 1 reply; 9+ messages in thread
From: Edwin Török @ 2023-02-24 13:36 UTC (permalink / raw
To: xen-devel; +Cc: Edwin Török, Wei Liu, Anthony PERARD, Juergen Gross
From: Edwin Török <edwin.torok@cloud.com>
From `man 2 realloc`:
`If realloc() fails, the original block is left untouched; it is not freed or moved.`
Found using GCC -fanalyzer:
```
| 184 | backup->entries = realloc(backup->entries,
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | | | |
| | | | (91) when ‘realloc’ fails
| | | (92) ‘old_ptes.entries’ leaks here; was allocated at (44)
| | (90) ...to here
```
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
---
tools/libs/guest/xg_offline_page.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/libs/guest/xg_offline_page.c b/tools/libs/guest/xg_offline_page.c
index c594fdba41..a8bcea768b 100644
--- a/tools/libs/guest/xg_offline_page.c
+++ b/tools/libs/guest/xg_offline_page.c
@@ -181,10 +181,13 @@ static int backup_ptes(xen_pfn_t table_mfn, int offset,
if (backup->max == backup->cur)
{
- backup->entries = realloc(backup->entries,
+ void* orig = backup->entries;
+ backup->entries = realloc(orig,
backup->max * 2 * sizeof(struct pte_backup_entry));
- if (backup->entries == NULL)
+ if (backup->entries == NULL) {
+ free(orig);
return -1;
+ }
else
backup->max *= 2;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
2023-02-24 13:36 ` [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak Edwin Török
@ 2023-02-24 14:56 ` Andrew Cooper
2023-02-27 10:36 ` Edwin Torok
2023-02-27 14:42 ` Juergen Gross
2023-02-27 15:00 ` [PATCH] libs/guest: Fix resource leaks in xc_core_arch_map_p2m_tree_rw() Andrew Cooper
1 sibling, 2 replies; 9+ messages in thread
From: Andrew Cooper @ 2023-02-24 14:56 UTC (permalink / raw
To: Edwin Török, xen-devel
Cc: Edwin Török, Wei Liu, Anthony PERARD, Juergen Gross
On 24/02/2023 1:36 pm, Edwin Török wrote:
> From: Edwin Török <edwin.torok@cloud.com>
>
> Prior to bd7a29c3d0 'out' would've always been executed and memory
> freed, but that commit changed it such that it returns early and leaks.
>
> Found using gcc 12.2.1 `-fanalyzer`:
> ```
> xg_core_x86.c: In function ‘xc_core_arch_map_p2m_tree_rw’:
> xg_core_x86.c:300:5: error: leak of ‘p2m_frame_list_list’ [CWE-401] [-Werror=analyzer-malloc-leak]
> 300 | return p2m_frame_list;
> | ^~~~~~
> ‘xc_core_arch_map_p2m_writable’: events 1-2
> |
> | 378 | xc_core_arch_map_p2m_writable(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (1) entry to ‘xc_core_arch_map_p2m_writable’
> |......
> | 381 | return xc_core_arch_map_p2m_rw(xch, dinfo, info, live_shinfo, live_p2m, 1);
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (2) calling ‘xc_core_arch_map_p2m_rw’ from ‘xc_core_arch_map_p2m_writable’
> |
> +--> ‘xc_core_arch_map_p2m_rw’: events 3-10
> |
> | 319 | xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
> | | ^~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (3) entry to ‘xc_core_arch_map_p2m_rw’
> |......
> | 328 | if ( xc_domain_nr_gpfns(xch, info->domid, &dinfo->p2m_size) < 0 )
> | | ~
> | | |
> | | (4) following ‘false’ branch...
> |......
> | 334 | if ( dinfo->p2m_size < info->nr_pages )
> | | ~~ ~
> | | | |
> | | | (6) following ‘false’ branch...
> | | (5) ...to here
> |......
> | 340 | p2m_cr3 = GET_FIELD(live_shinfo, arch.p2m_cr3, dinfo->guest_width);
> | | ~~~~~~~
> | | |
> | | (7) ...to here
> | 341 |
> | 342 | p2m_frame_list = p2m_cr3 ? xc_core_arch_map_p2m_list_rw(xch, dinfo, dom, live_shinfo, p2m_cr3)
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | 343 | : xc_core_arch_map_p2m_tree_rw(xch, dinfo, dom, live_shinfo);
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | | |
> | | | (9) ...to here
> | | | (10) calling ‘xc_core_arch_map_p2m_tree_rw’ from ‘xc_core_arch_map_p2m_rw’
> | | (8) following ‘false’ branch...
> |
> +--> ‘xc_core_arch_map_p2m_tree_rw’: events 11-24
> |
> | 228 | xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinfo,
> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (11) entry to ‘xc_core_arch_map_p2m_tree_rw’
> |......
> | 245 | if ( !live_p2m_frame_list_list )
> | | ~
> | | |
> | | (12) following ‘false’ branch (when ‘live_p2m_frame_list_list’ is non-NULL)...
> |......
> | 252 | if ( !(p2m_frame_list_list = malloc(PAGE_SIZE)) )
> | | ~~ ~ ~~~~~~~~~~~~~~~~~
> | | | | |
> | | | | (14) allocated here
> | | | (15) assuming ‘p2m_frame_list_list’ is non-NULL
> | | | (16) following ‘false’ branch (when ‘p2m_frame_list_list’ is non-NULL)...
> | | (13) ...to here
> |......
> | 257 | memcpy(p2m_frame_list_list, live_p2m_frame_list_list, PAGE_SIZE);
> | | ~~~~~~
> | | |
> | | (17) ...to here
> |......
> | 266 | else if ( dinfo->guest_width < sizeof(unsigned long) )
> | | ~
> | | |
> | | (18) following ‘false’ branch...
> |......
> | 270 | live_p2m_frame_list =
> | | ~~~~~~~~~~~~~~~~~~~
> | | |
> | | (19) ...to here
> |......
> | 275 | if ( !live_p2m_frame_list )
> | | ~
> | | |
> | | (20) following ‘false’ branch (when ‘live_p2m_frame_list’ is non-NULL)...
> |......
> | 282 | if ( !(p2m_frame_list = malloc(P2M_TOOLS_FL_SIZE)) )
> | | ~~ ~
> | | | |
> | | | (22) following ‘false’ branch (when ‘p2m_frame_list’ is non-NULL)...
> | | (21) ...to here
> |......
> | 287 | memset(p2m_frame_list, 0, P2M_TOOLS_FL_SIZE);
> | | ~~~~~~
> | | |
> | | (23) ...to here
> |......
> | 300 | return p2m_frame_list;
> | | ~~~~~~
> | | |
> | | (24) ‘p2m_frame_list_list’ leaks here; was allocated at (14)
> |
> ```
> Fixes: bd7a29c3d0 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
>
> Signed-off-by: Edwin Török <edwin.torok@cloud.com>
> ---
> tools/libs/guest/xg_core_x86.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
> index 61106b98b8..69929879d7 100644
> --- a/tools/libs/guest/xg_core_x86.c
> +++ b/tools/libs/guest/xg_core_x86.c
> @@ -297,6 +297,8 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinf
>
> dinfo->p2m_frames = P2M_FL_ENTRIES;
>
> + free(p2m_frame_list_list);
> +
> return p2m_frame_list;
>
> out:
I agree there are problems here, but I think they're larger still. The
live_p2m_frame_list_list and live_p2m_frame_list foreign mappings are
leaked too on the success path.
I think this is the necessary fix:
~Andrew
----8<----
diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
index 61106b98b877..c5e4542ccccc 100644
--- a/tools/libs/guest/xg_core_x86.c
+++ b/tools/libs/guest/xg_core_x86.c
@@ -229,11 +229,11 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
struct domain_info_context *dinf
uint32_t dom, shared_info_any_t *live_shinfo)
{
/* Double and single indirect references to the live P2M table */
- xen_pfn_t *live_p2m_frame_list_list;
+ xen_pfn_t *live_p2m_frame_list_list = NULL;
xen_pfn_t *live_p2m_frame_list = NULL;
/* Copies of the above. */
xen_pfn_t *p2m_frame_list_list = NULL;
- xen_pfn_t *p2m_frame_list;
+ xen_pfn_t *p2m_frame_list = NULL;
int err;
int i;
@@ -297,8 +297,6 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
struct domain_info_context *dinf
dinfo->p2m_frames = P2M_FL_ENTRIES;
- return p2m_frame_list;
-
out:
err = errno;
@@ -312,7 +310,7 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
struct domain_info_context *dinf
errno = err;
- return NULL;
+ return p2m_frame_list;
}
static int
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] backup_ptes: fix leak on realloc failure
2023-02-24 13:36 ` [PATCH v1 2/2] backup_ptes: fix leak on realloc failure Edwin Török
@ 2023-02-24 15:00 ` Andrew Cooper
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Cooper @ 2023-02-24 15:00 UTC (permalink / raw
To: Edwin Török, xen-devel
Cc: Edwin Török, Wei Liu, Anthony PERARD, Juergen Gross
On 24/02/2023 1:36 pm, Edwin Török wrote:
> From: Edwin Török <edwin.torok@cloud.com>
>
> From `man 2 realloc`:
> `If realloc() fails, the original block is left untouched; it is not freed or moved.`
>
> Found using GCC -fanalyzer:
> ```
> | 184 | backup->entries = realloc(backup->entries,
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | | | |
> | | | | (91) when ‘realloc’ fails
> | | | (92) ‘old_ptes.entries’ leaks here; was allocated at (44)
> | | (90) ...to here
> ```
>
> Signed-off-by: Edwin Török <edwin.torok@cloud.com>
In terms of the fix, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>, but
> ---
> tools/libs/guest/xg_offline_page.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libs/guest/xg_offline_page.c b/tools/libs/guest/xg_offline_page.c
> index c594fdba41..a8bcea768b 100644
> --- a/tools/libs/guest/xg_offline_page.c
> +++ b/tools/libs/guest/xg_offline_page.c
> @@ -181,10 +181,13 @@ static int backup_ptes(xen_pfn_t table_mfn, int offset,
>
> if (backup->max == backup->cur)
> {
> - backup->entries = realloc(backup->entries,
> + void* orig = backup->entries;
void *orig, and a newline.
> + backup->entries = realloc(orig,
> backup->max * 2 * sizeof(struct pte_backup_entry));
> - if (backup->entries == NULL)
> + if (backup->entries == NULL) {
Newline.
Can be fixed on commit.
~Andrew
> + free(orig);
> return -1;
> + }
> else
> backup->max *= 2;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
2023-02-24 14:56 ` Andrew Cooper
@ 2023-02-27 10:36 ` Edwin Torok
2023-02-27 14:42 ` Juergen Gross
1 sibling, 0 replies; 9+ messages in thread
From: Edwin Torok @ 2023-02-27 10:36 UTC (permalink / raw
To: Andrew Cooper
Cc: Edwin Török, xen-devel, Wei Liu, Anthony PERARD,
Juergen Gross
> On 24 Feb 2023, at 14:56, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>
> On 24/02/2023 1:36 pm, Edwin Török wrote:
>> From: Edwin Török <edwin.torok@cloud.com>
>>
>> Prior to bd7a29c3d0 'out' would've always been executed and memory
>> freed, but that commit changed it such that it returns early and leaks.
>>
>> Found using gcc 12.2.1 `-fanalyzer`:
>> ```
>> xg_core_x86.c: In function ‘xc_core_arch_map_p2m_tree_rw’:
>> xg_core_x86.c:300:5: error: leak of ‘p2m_frame_list_list’ [CWE-401] [-Werror=analyzer-malloc-leak]
>> 300 | return p2m_frame_list;
>> | ^~~~~~
>> ‘xc_core_arch_map_p2m_writable’: events 1-2
>> |
>> | 378 | xc_core_arch_map_p2m_writable(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (1) entry to ‘xc_core_arch_map_p2m_writable’
>> |......
>> | 381 | return xc_core_arch_map_p2m_rw(xch, dinfo, info, live_shinfo, live_p2m, 1);
>> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (2) calling ‘xc_core_arch_map_p2m_rw’ from ‘xc_core_arch_map_p2m_writable’
>> |
>> +--> ‘xc_core_arch_map_p2m_rw’: events 3-10
>> |
>> | 319 | xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
>> | | ^~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (3) entry to ‘xc_core_arch_map_p2m_rw’
>> |......
>> | 328 | if ( xc_domain_nr_gpfns(xch, info->domid, &dinfo->p2m_size) < 0 )
>> | | ~
>> | | |
>> | | (4) following ‘false’ branch...
>> |......
>> | 334 | if ( dinfo->p2m_size < info->nr_pages )
>> | | ~~ ~
>> | | | |
>> | | | (6) following ‘false’ branch...
>> | | (5) ...to here
>> |......
>> | 340 | p2m_cr3 = GET_FIELD(live_shinfo, arch.p2m_cr3, dinfo->guest_width);
>> | | ~~~~~~~
>> | | |
>> | | (7) ...to here
>> | 341 |
>> | 342 | p2m_frame_list = p2m_cr3 ? xc_core_arch_map_p2m_list_rw(xch, dinfo, dom, live_shinfo, p2m_cr3)
>> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | 343 | : xc_core_arch_map_p2m_tree_rw(xch, dinfo, dom, live_shinfo);
>> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | | |
>> | | | (9) ...to here
>> | | | (10) calling ‘xc_core_arch_map_p2m_tree_rw’ from ‘xc_core_arch_map_p2m_rw’
>> | | (8) following ‘false’ branch...
>> |
>> +--> ‘xc_core_arch_map_p2m_tree_rw’: events 11-24
>> |
>> | 228 | xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinfo,
>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (11) entry to ‘xc_core_arch_map_p2m_tree_rw’
>> |......
>> | 245 | if ( !live_p2m_frame_list_list )
>> | | ~
>> | | |
>> | | (12) following ‘false’ branch (when ‘live_p2m_frame_list_list’ is non-NULL)...
>> |......
>> | 252 | if ( !(p2m_frame_list_list = malloc(PAGE_SIZE)) )
>> | | ~~ ~ ~~~~~~~~~~~~~~~~~
>> | | | | |
>> | | | | (14) allocated here
>> | | | (15) assuming ‘p2m_frame_list_list’ is non-NULL
>> | | | (16) following ‘false’ branch (when ‘p2m_frame_list_list’ is non-NULL)...
>> | | (13) ...to here
>> |......
>> | 257 | memcpy(p2m_frame_list_list, live_p2m_frame_list_list, PAGE_SIZE);
>> | | ~~~~~~
>> | | |
>> | | (17) ...to here
>> |......
>> | 266 | else if ( dinfo->guest_width < sizeof(unsigned long) )
>> | | ~
>> | | |
>> | | (18) following ‘false’ branch...
>> |......
>> | 270 | live_p2m_frame_list =
>> | | ~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (19) ...to here
>> |......
>> | 275 | if ( !live_p2m_frame_list )
>> | | ~
>> | | |
>> | | (20) following ‘false’ branch (when ‘live_p2m_frame_list’ is non-NULL)...
>> |......
>> | 282 | if ( !(p2m_frame_list = malloc(P2M_TOOLS_FL_SIZE)) )
>> | | ~~ ~
>> | | | |
>> | | | (22) following ‘false’ branch (when ‘p2m_frame_list’ is non-NULL)...
>> | | (21) ...to here
>> |......
>> | 287 | memset(p2m_frame_list, 0, P2M_TOOLS_FL_SIZE);
>> | | ~~~~~~
>> | | |
>> | | (23) ...to here
>> |......
>> | 300 | return p2m_frame_list;
>> | | ~~~~~~
>> | | |
>> | | (24) ‘p2m_frame_list_list’ leaks here; was allocated at (14)
>> |
>> ```
>> Fixes: bd7a29c3d0 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
>>
>> Signed-off-by: Edwin Török <edwin.torok@cloud.com>
>> ---
>> tools/libs/guest/xg_core_x86.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
>> index 61106b98b8..69929879d7 100644
>> --- a/tools/libs/guest/xg_core_x86.c
>> +++ b/tools/libs/guest/xg_core_x86.c
>> @@ -297,6 +297,8 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinf
>>
>> dinfo->p2m_frames = P2M_FL_ENTRIES;
>>
>> + free(p2m_frame_list_list);
>> +
>> return p2m_frame_list;
>>
>> out:
>
> I agree there are problems here, but I think they're larger still. The
> live_p2m_frame_list_list and live_p2m_frame_list foreign mappings are
> leaked too on the success path.
I thought the goal of that function was to create the mapping (judging by its name), but looking at its caller there is another map_foreign_pages there,
so there is indeed a leak (-fanalyzer won't be able to spot these unless we figure out a way to put some attributs on these map and unmap to teach it that they are alloc/free pairs).
Pushed updated commits here (top 2): leak-fixes <https://github.com/edwintorok/xen/commits/leak-fixes>
Before posting a V2 I'll try it out on an actual machine though, just to check that we don't have a double-free instead now.
Thanks,
--Edwin
>
> I think this is the necessary fix:
>
> ~Andrew
>
> ----8<----
>
> diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
> index 61106b98b877..c5e4542ccccc 100644
> --- a/tools/libs/guest/xg_core_x86.c
> +++ b/tools/libs/guest/xg_core_x86.c
> @@ -229,11 +229,11 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
> struct domain_info_context *dinf
> uint32_t dom, shared_info_any_t *live_shinfo)
> {
> /* Double and single indirect references to the live P2M table */
> - xen_pfn_t *live_p2m_frame_list_list;
> + xen_pfn_t *live_p2m_frame_list_list = NULL;
> xen_pfn_t *live_p2m_frame_list = NULL;
> /* Copies of the above. */
> xen_pfn_t *p2m_frame_list_list = NULL;
> - xen_pfn_t *p2m_frame_list;
> + xen_pfn_t *p2m_frame_list = NULL;
>
> int err;
> int i;
> @@ -297,8 +297,6 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
> struct domain_info_context *dinf
>
> dinfo->p2m_frames = P2M_FL_ENTRIES;
>
> - return p2m_frame_list;
> -
> out:
> err = errno;
>
> @@ -312,7 +310,7 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
> struct domain_info_context *dinf
>
> errno = err;
>
> - return NULL;
> + return p2m_frame_list;
> }
>
> static int
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
2023-02-24 14:56 ` Andrew Cooper
2023-02-27 10:36 ` Edwin Torok
@ 2023-02-27 14:42 ` Juergen Gross
2023-02-27 14:49 ` Andrew Cooper
1 sibling, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2023-02-27 14:42 UTC (permalink / raw
To: Andrew Cooper, Edwin Török, xen-devel
Cc: Edwin Török, Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 9063 bytes --]
On 24.02.23 15:56, Andrew Cooper wrote:
> On 24/02/2023 1:36 pm, Edwin Török wrote:
>> From: Edwin Török <edwin.torok@cloud.com>
>>
>> Prior to bd7a29c3d0 'out' would've always been executed and memory
>> freed, but that commit changed it such that it returns early and leaks.
>>
>> Found using gcc 12.2.1 `-fanalyzer`:
>> ```
>> xg_core_x86.c: In function ‘xc_core_arch_map_p2m_tree_rw’:
>> xg_core_x86.c:300:5: error: leak of ‘p2m_frame_list_list’ [CWE-401] [-Werror=analyzer-malloc-leak]
>> 300 | return p2m_frame_list;
>> | ^~~~~~
>> ‘xc_core_arch_map_p2m_writable’: events 1-2
>> |
>> | 378 | xc_core_arch_map_p2m_writable(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (1) entry to ‘xc_core_arch_map_p2m_writable’
>> |......
>> | 381 | return xc_core_arch_map_p2m_rw(xch, dinfo, info, live_shinfo, live_p2m, 1);
>> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (2) calling ‘xc_core_arch_map_p2m_rw’ from ‘xc_core_arch_map_p2m_writable’
>> |
>> +--> ‘xc_core_arch_map_p2m_rw’: events 3-10
>> |
>> | 319 | xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc_dominfo_t *info,
>> | | ^~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (3) entry to ‘xc_core_arch_map_p2m_rw’
>> |......
>> | 328 | if ( xc_domain_nr_gpfns(xch, info->domid, &dinfo->p2m_size) < 0 )
>> | | ~
>> | | |
>> | | (4) following ‘false’ branch...
>> |......
>> | 334 | if ( dinfo->p2m_size < info->nr_pages )
>> | | ~~ ~
>> | | | |
>> | | | (6) following ‘false’ branch...
>> | | (5) ...to here
>> |......
>> | 340 | p2m_cr3 = GET_FIELD(live_shinfo, arch.p2m_cr3, dinfo->guest_width);
>> | | ~~~~~~~
>> | | |
>> | | (7) ...to here
>> | 341 |
>> | 342 | p2m_frame_list = p2m_cr3 ? xc_core_arch_map_p2m_list_rw(xch, dinfo, dom, live_shinfo, p2m_cr3)
>> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | 343 | : xc_core_arch_map_p2m_tree_rw(xch, dinfo, dom, live_shinfo);
>> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | | |
>> | | | (9) ...to here
>> | | | (10) calling ‘xc_core_arch_map_p2m_tree_rw’ from ‘xc_core_arch_map_p2m_rw’
>> | | (8) following ‘false’ branch...
>> |
>> +--> ‘xc_core_arch_map_p2m_tree_rw’: events 11-24
>> |
>> | 228 | xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinfo,
>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (11) entry to ‘xc_core_arch_map_p2m_tree_rw’
>> |......
>> | 245 | if ( !live_p2m_frame_list_list )
>> | | ~
>> | | |
>> | | (12) following ‘false’ branch (when ‘live_p2m_frame_list_list’ is non-NULL)...
>> |......
>> | 252 | if ( !(p2m_frame_list_list = malloc(PAGE_SIZE)) )
>> | | ~~ ~ ~~~~~~~~~~~~~~~~~
>> | | | | |
>> | | | | (14) allocated here
>> | | | (15) assuming ‘p2m_frame_list_list’ is non-NULL
>> | | | (16) following ‘false’ branch (when ‘p2m_frame_list_list’ is non-NULL)...
>> | | (13) ...to here
>> |......
>> | 257 | memcpy(p2m_frame_list_list, live_p2m_frame_list_list, PAGE_SIZE);
>> | | ~~~~~~
>> | | |
>> | | (17) ...to here
>> |......
>> | 266 | else if ( dinfo->guest_width < sizeof(unsigned long) )
>> | | ~
>> | | |
>> | | (18) following ‘false’ branch...
>> |......
>> | 270 | live_p2m_frame_list =
>> | | ~~~~~~~~~~~~~~~~~~~
>> | | |
>> | | (19) ...to here
>> |......
>> | 275 | if ( !live_p2m_frame_list )
>> | | ~
>> | | |
>> | | (20) following ‘false’ branch (when ‘live_p2m_frame_list’ is non-NULL)...
>> |......
>> | 282 | if ( !(p2m_frame_list = malloc(P2M_TOOLS_FL_SIZE)) )
>> | | ~~ ~
>> | | | |
>> | | | (22) following ‘false’ branch (when ‘p2m_frame_list’ is non-NULL)...
>> | | (21) ...to here
>> |......
>> | 287 | memset(p2m_frame_list, 0, P2M_TOOLS_FL_SIZE);
>> | | ~~~~~~
>> | | |
>> | | (23) ...to here
>> |......
>> | 300 | return p2m_frame_list;
>> | | ~~~~~~
>> | | |
>> | | (24) ‘p2m_frame_list_list’ leaks here; was allocated at (14)
>> |
>> ```
>> Fixes: bd7a29c3d0 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
>>
>> Signed-off-by: Edwin Török <edwin.torok@cloud.com>
>> ---
>> tools/libs/guest/xg_core_x86.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
>> index 61106b98b8..69929879d7 100644
>> --- a/tools/libs/guest/xg_core_x86.c
>> +++ b/tools/libs/guest/xg_core_x86.c
>> @@ -297,6 +297,8 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinf
>>
>> dinfo->p2m_frames = P2M_FL_ENTRIES;
>>
>> + free(p2m_frame_list_list);
>> +
>> return p2m_frame_list;
>>
>> out:
>
> I agree there are problems here, but I think they're larger still. The
> live_p2m_frame_list_list and live_p2m_frame_list foreign mappings are
> leaked too on the success path.
>
> I think this is the necessary fix:
Yes, I agree.
>
> ~Andrew
>
> ----8<----
>
> diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
> index 61106b98b877..c5e4542ccccc 100644
> --- a/tools/libs/guest/xg_core_x86.c
> +++ b/tools/libs/guest/xg_core_x86.c
> @@ -229,11 +229,11 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
> struct domain_info_context *dinf
> uint32_t dom, shared_info_any_t *live_shinfo)
> {
> /* Double and single indirect references to the live P2M table */
> - xen_pfn_t *live_p2m_frame_list_list;
> + xen_pfn_t *live_p2m_frame_list_list = NULL;
> xen_pfn_t *live_p2m_frame_list = NULL;
> /* Copies of the above. */
> xen_pfn_t *p2m_frame_list_list = NULL;
> - xen_pfn_t *p2m_frame_list;
> + xen_pfn_t *p2m_frame_list = NULL;
>
> int err;
> int i;
> @@ -297,8 +297,6 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
> struct domain_info_context *dinf
>
> dinfo->p2m_frames = P2M_FL_ENTRIES;
>
> - return p2m_frame_list;
> -
> out:
> err = errno;
>
> @@ -312,7 +310,7 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
> struct domain_info_context *dinf
>
> errno = err;
>
> - return NULL;
> + return p2m_frame_list;
> }
>
> static int
>
In case this fix is taken, my
Reviewed-by: Juergen Gross <jgross@suse.com>
can be added.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
2023-02-27 14:42 ` Juergen Gross
@ 2023-02-27 14:49 ` Andrew Cooper
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Cooper @ 2023-02-27 14:49 UTC (permalink / raw
To: Juergen Gross, Edwin Török, xen-devel
Cc: Edwin Török, Wei Liu, Anthony PERARD
On 27/02/2023 2:42 pm, Juergen Gross wrote:
> On 24.02.23 15:56, Andrew Cooper wrote:
>> On 24/02/2023 1:36 pm, Edwin Török wrote:
>>> From: Edwin Török <edwin.torok@cloud.com>
>>>
>>> Prior to bd7a29c3d0 'out' would've always been executed and memory
>>> freed, but that commit changed it such that it returns early and leaks.
>>>
>>> Found using gcc 12.2.1 `-fanalyzer`:
>>> ```
>>> xg_core_x86.c: In function ‘xc_core_arch_map_p2m_tree_rw’:
>>> xg_core_x86.c:300:5: error: leak of ‘p2m_frame_list_list’ [CWE-401]
>>> [-Werror=analyzer-malloc-leak]
>>> 300 | return p2m_frame_list;
>>> | ^~~~~~
>>> ‘xc_core_arch_map_p2m_writable’: events 1-2
>>> |
>>> | 378 | xc_core_arch_map_p2m_writable(xc_interface *xch,
>>> struct domain_info_context *dinfo, xc_dominfo_t *info,
>>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> | | |
>>> | | (1) entry to ‘xc_core_arch_map_p2m_writable’
>>> |......
>>> | 381 | return xc_core_arch_map_p2m_rw(xch, dinfo, info,
>>> live_shinfo, live_p2m, 1);
>>> | |
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> | | |
>>> | | (2) calling ‘xc_core_arch_map_p2m_rw’ from
>>> ‘xc_core_arch_map_p2m_writable’
>>> |
>>> +--> ‘xc_core_arch_map_p2m_rw’: events 3-10
>>> |
>>> | 319 | xc_core_arch_map_p2m_rw(xc_interface *xch,
>>> struct domain_info_context *dinfo, xc_dominfo_t *info,
>>> | | ^~~~~~~~~~~~~~~~~~~~~~~
>>> | | |
>>> | | (3) entry to ‘xc_core_arch_map_p2m_rw’
>>> |......
>>> | 328 | if ( xc_domain_nr_gpfns(xch, info->domid,
>>> &dinfo->p2m_size) < 0 )
>>> | | ~
>>> | | |
>>> | | (4) following ‘false’ branch...
>>> |......
>>> | 334 | if ( dinfo->p2m_size < info->nr_pages )
>>> | | ~~ ~
>>> | | | |
>>> | | | (6) following ‘false’ branch...
>>> | | (5) ...to here
>>> |......
>>> | 340 | p2m_cr3 = GET_FIELD(live_shinfo,
>>> arch.p2m_cr3, dinfo->guest_width);
>>> | | ~~~~~~~
>>> | | |
>>> | | (7) ...to here
>>> | 341 |
>>> | 342 | p2m_frame_list = p2m_cr3 ?
>>> xc_core_arch_map_p2m_list_rw(xch, dinfo, dom, live_shinfo, p2m_cr3)
>>> | |
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> | 343 | :
>>> xc_core_arch_map_p2m_tree_rw(xch, dinfo, dom, live_shinfo);
>>> | |
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> | | | |
>>> | | | (9) ...to here
>>> | | | (10) calling
>>> ‘xc_core_arch_map_p2m_tree_rw’ from ‘xc_core_arch_map_p2m_rw’
>>> | | (8) following
>>> ‘false’ branch...
>>> |
>>> +--> ‘xc_core_arch_map_p2m_tree_rw’: events 11-24
>>> |
>>> | 228 |
>>> xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct
>>> domain_info_context *dinfo,
>>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> | | |
>>> | | (11) entry to
>>> ‘xc_core_arch_map_p2m_tree_rw’
>>> |......
>>> | 245 | if ( !live_p2m_frame_list_list )
>>> | | ~
>>> | | |
>>> | | (12) following ‘false’ branch
>>> (when ‘live_p2m_frame_list_list’ is non-NULL)...
>>> |......
>>> | 252 | if ( !(p2m_frame_list_list =
>>> malloc(PAGE_SIZE)) )
>>> | | ~~ ~
>>> ~~~~~~~~~~~~~~~~~
>>> | | | | |
>>> | | | | (14)
>>> allocated here
>>> | | | (15) assuming
>>> ‘p2m_frame_list_list’ is non-NULL
>>> | | | (16) following ‘false’ branch
>>> (when ‘p2m_frame_list_list’ is non-NULL)...
>>> | | (13) ...to here
>>> |......
>>> | 257 | memcpy(p2m_frame_list_list,
>>> live_p2m_frame_list_list, PAGE_SIZE);
>>> | | ~~~~~~
>>> | | |
>>> | | (17) ...to here
>>> |......
>>> | 266 | else if ( dinfo->guest_width <
>>> sizeof(unsigned long) )
>>> | | ~
>>> | | |
>>> | | (18) following ‘false’
>>> branch...
>>> |......
>>> | 270 | live_p2m_frame_list =
>>> | | ~~~~~~~~~~~~~~~~~~~
>>> | | |
>>> | | (19) ...to here
>>> |......
>>> | 275 | if ( !live_p2m_frame_list )
>>> | | ~
>>> | | |
>>> | | (20) following ‘false’ branch
>>> (when ‘live_p2m_frame_list’ is non-NULL)...
>>> |......
>>> | 282 | if ( !(p2m_frame_list =
>>> malloc(P2M_TOOLS_FL_SIZE)) )
>>> | | ~~ ~
>>> | | | |
>>> | | | (22) following ‘false’ branch
>>> (when ‘p2m_frame_list’ is non-NULL)...
>>> | | (21) ...to here
>>> |......
>>> | 287 | memset(p2m_frame_list, 0,
>>> P2M_TOOLS_FL_SIZE);
>>> | | ~~~~~~
>>> | | |
>>> | | (23) ...to here
>>> |......
>>> | 300 | return p2m_frame_list;
>>> | | ~~~~~~
>>> | | |
>>> | | (24) ‘p2m_frame_list_list’ leaks
>>> here; was allocated at (14)
>>> |
>>> ```
>>> Fixes: bd7a29c3d0 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to
>>> support linear p2m table")
>>>
>>> Signed-off-by: Edwin Török <edwin.torok@cloud.com>
>>> ---
>>> tools/libs/guest/xg_core_x86.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/tools/libs/guest/xg_core_x86.c
>>> b/tools/libs/guest/xg_core_x86.c
>>> index 61106b98b8..69929879d7 100644
>>> --- a/tools/libs/guest/xg_core_x86.c
>>> +++ b/tools/libs/guest/xg_core_x86.c
>>> @@ -297,6 +297,8 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
>>> struct domain_info_context *dinf
>>> dinfo->p2m_frames = P2M_FL_ENTRIES;
>>> + free(p2m_frame_list_list);
>>> +
>>> return p2m_frame_list;
>>> out:
>>
>> I agree there are problems here, but I think they're larger still. The
>> live_p2m_frame_list_list and live_p2m_frame_list foreign mappings are
>> leaked too on the success path.
>>
>> I think this is the necessary fix:
>
> Yes, I agree.
>
>>
>> ~Andrew
>>
>> ----8<----
>>
>> diff --git a/tools/libs/guest/xg_core_x86.c
>> b/tools/libs/guest/xg_core_x86.c
>> index 61106b98b877..c5e4542ccccc 100644
>> --- a/tools/libs/guest/xg_core_x86.c
>> +++ b/tools/libs/guest/xg_core_x86.c
>> @@ -229,11 +229,11 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
>> struct domain_info_context *dinf
>> uint32_t dom, shared_info_any_t
>> *live_shinfo)
>> {
>> /* Double and single indirect references to the live P2M table */
>> - xen_pfn_t *live_p2m_frame_list_list;
>> + xen_pfn_t *live_p2m_frame_list_list = NULL;
>> xen_pfn_t *live_p2m_frame_list = NULL;
>> /* Copies of the above. */
>> xen_pfn_t *p2m_frame_list_list = NULL;
>> - xen_pfn_t *p2m_frame_list;
>> + xen_pfn_t *p2m_frame_list = NULL;
>> int err;
>> int i;
>> @@ -297,8 +297,6 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
>> struct domain_info_context *dinf
>> dinfo->p2m_frames = P2M_FL_ENTRIES;
>> - return p2m_frame_list;
>> -
>> out:
>> err = errno;
>> @@ -312,7 +310,7 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch,
>> struct domain_info_context *dinf
>> errno = err;
>> - return NULL;
>> + return p2m_frame_list;
>> }
>> static int
>>
>
> In case this fix is taken, my
>
> Reviewed-by: Juergen Gross <jgross@suse.com>
>
> can be added.
Thanks. I'll write a full patch and post it, with appropriate tags, and
also include it in my commit sweep.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] libs/guest: Fix resource leaks in xc_core_arch_map_p2m_tree_rw()
2023-02-24 13:36 ` [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak Edwin Török
2023-02-24 14:56 ` Andrew Cooper
@ 2023-02-27 15:00 ` Andrew Cooper
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Cooper @ 2023-02-27 15:00 UTC (permalink / raw
To: Xen-devel
Cc: Edwin Török, Andrew Cooper, Edwin Török,
Juergen Gross
Edwin, with the help of GCC's -fanalyzer, identified that p2m_frame_list_list
gets leaked. What fanalyzer can't see is that the live_p2m_frame_list_list
and live_p2m_frame_list foreign mappings are leaked too.
Rework the logic so the out path is executed unconditionally, which cleans up
all the intermediate allocations/mappings appropriately.
Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
Reported-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
tools/libs/guest/xg_core_x86.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
index 61106b98b877..c5e4542ccccc 100644
--- a/tools/libs/guest/xg_core_x86.c
+++ b/tools/libs/guest/xg_core_x86.c
@@ -229,11 +229,11 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinf
uint32_t dom, shared_info_any_t *live_shinfo)
{
/* Double and single indirect references to the live P2M table */
- xen_pfn_t *live_p2m_frame_list_list;
+ xen_pfn_t *live_p2m_frame_list_list = NULL;
xen_pfn_t *live_p2m_frame_list = NULL;
/* Copies of the above. */
xen_pfn_t *p2m_frame_list_list = NULL;
- xen_pfn_t *p2m_frame_list;
+ xen_pfn_t *p2m_frame_list = NULL;
int err;
int i;
@@ -297,8 +297,6 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinf
dinfo->p2m_frames = P2M_FL_ENTRIES;
- return p2m_frame_list;
-
out:
err = errno;
@@ -312,7 +310,7 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct domain_info_context *dinf
errno = err;
- return NULL;
+ return p2m_frame_list;
}
static int
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-02-27 15:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-24 13:36 [PATCH v1 0/2] fix memory leaks reported by GCC -fanalyzer Edwin Török
2023-02-24 13:36 ` [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak Edwin Török
2023-02-24 14:56 ` Andrew Cooper
2023-02-27 10:36 ` Edwin Torok
2023-02-27 14:42 ` Juergen Gross
2023-02-27 14:49 ` Andrew Cooper
2023-02-27 15:00 ` [PATCH] libs/guest: Fix resource leaks in xc_core_arch_map_p2m_tree_rw() Andrew Cooper
2023-02-24 13:36 ` [PATCH v1 2/2] backup_ptes: fix leak on realloc failure Edwin Török
2023-02-24 15:00 ` Andrew Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).