| Message ID | fe8cb20f-8541-19a9-a0cf-06e9206b1299@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools: address Coverity UNUSED issues | expand |
On 12.06.23 13:46, 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@suse.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
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@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().
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@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 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. Jan
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@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. Cheers,
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 <jbeulich@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
--- 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); }
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@suse.com>