Re: [PATCH 3/5] libxg: drop dead assignment to "ptes" from xc_core_arch_map_p2m_list_rw()

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Jun 13, 2023 at 06:08:16PM +0200, Jan Beulich wrote:
> On 13.06.2023 18:03, Anthony PERARD wrote:
> > On Mon, Jun 12, 2023 at 01:46:40PM +0200, Jan Beulich wrote:
> >> The function returns immediately after the enclosing if().
> >>
> >> Coverity ID: 1532314
> >> Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to 
> >> support linear p2m table")
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> --- a/tools/libs/guest/xg_core_x86.c
> >> +++ b/tools/libs/guest/xg_core_x86.c
> >> @@ -210,7 +210,6 @@ xc_core_arch_map_p2m_list_rw(xc_interfac
> >>          }
> >>  
> >>          munmap(ptes, n_pages * PAGE_SIZE);
> >> -        ptes = NULL;
> >>          off = p2m_vaddr & ((mask >> shift) << shift);
> >>      }
> > 
> > Do we have to remove this assignment? What if someone adds code later
> > and reuse the content of the variable `ptes`? Or what if someone adds
> > codes after the loop, and handle an error with `goto out`, we would have
> > a double-munmap().
> 
> Imo it would be at that time when the assignment wants (re)adding. I

I don't believe this is going to happen because I don't think a compiler
will find a mistake. Maybe a run of Coverity would tell that ptes is
reused after munmap(), but by the time Coverity run on the code, it
would be too late.

> fully agree with the tool, and I expect Misra (if it was applied to
> the tool stack as well) would demand the very same change.

I guess the issue here is that there's two paths out of the function, the
error path via "out" and the success path. If `ptes` is check on both
path, then the assignment would be needed, and it would be harder to
make a mistake; which can be done by having only one way out.

If only we could restrict the scope of `ptes` to the for loop, we
wouldn't even need to set it to NULL.

Cheers,

-- 
Anthony PERARD