Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
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 >>> >>> 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, >>> >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) ) >>>
Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
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 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, >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 )
Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
> On 24 Feb 2023, at 14:56, Andrew Cooper wrote: > > On 24/02/2023 1:36 pm, Edwin Török wrote: >> From: Edwin Török >> >> 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, >> >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 = >> | |
Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
On 24/02/2023 1:36 pm, Edwin Török wrote: > From: Edwin Török > > 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, > >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
[PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
From: Edwin Török 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, >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
