* [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
* 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 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
* [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 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
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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.