On 13.06.2023 18:40, Anthony PERARD wrote:
> 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 <jbeul...@suse.com>
>>>>
>>>> --- 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.
I can do that, but then I can't resist to shrink other variables' scopes
as well, so it'll be somewhat bigger a change. Let's see how you like it.
Jan
