| Message ID | 2d1335a4056558d172d9aa3e59982eb761647418.1640590794.git.ehem+xen@m5p.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some misc from my tree | expand |
> On 10 Dec 2020, at 23:09, Elliott Mitchell <ehem+xen@m5p.com> wrote: > > There is potential for the info and info_free variable's purposes to > diverge. If info was overwritten with a distinct value, yet info_free > still needed deallocation a bug would occur on this line. Preemptively > address this issue (making use of divergent info/info_free values is > under consideration). > Looks ok to me Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> > --- > tools/xl/xl_info.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c > index 3647468420..938f06f1a8 100644 > --- a/tools/xl/xl_info.c > +++ b/tools/xl/xl_info.c > @@ -579,7 +579,7 @@ int main_list(int argc, char **argv) > info, nb_domain); > > if (info_free) > - libxl_dominfo_list_free(info, nb_domain); > + libxl_dominfo_list_free(info_free, nb_domain); > > libxl_dominfo_dispose(&info_buf); > > -- > 2.30.2 > >
On Thu, Dec 10, 2020 at 03:09:06PM -0800, Elliott Mitchell wrote: > There is potential for the info and info_free variable's purposes to > diverge. If info was overwritten with a distinct value, yet info_free > still needed deallocation a bug would occur on this line. Preemptively > address this issue (making use of divergent info/info_free values is > under consideration). > > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> > --- > tools/xl/xl_info.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c > index 3647468420..938f06f1a8 100644 > --- a/tools/xl/xl_info.c > +++ b/tools/xl/xl_info.c > @@ -579,7 +579,7 @@ int main_list(int argc, char **argv) > info, nb_domain); > > if (info_free) > - libxl_dominfo_list_free(info, nb_domain); > + libxl_dominfo_list_free(info_free, nb_domain); > > libxl_dominfo_dispose(&info_buf); > I don't think this is the right thing to do with this patch. libxl_dominfo_list_free() should use the same variable that is used by libxl_list_domain(). What we want to free is the allocation made by libxl_list_domain(). "info_free" in the function seems to be used as a boolean which tell if "info" have been allocated or not. Actually, it probably say if "info" is a list of "libxl_dominfo" or not. So instead of just replacing "info" by "info_free" here, we should instead store the result from libxl_list_domain() into a different variable and free that, like it is done with "info_buf". I hope that makes sense? Thanks,
Huh, never got around to replying to this. Too many things going on, too many distractions... On Wed, Jan 05, 2022 at 03:05:23PM +0000, Anthony PERARD wrote: > On Thu, Dec 10, 2020 at 03:09:06PM -0800, Elliott Mitchell wrote: > > There is potential for the info and info_free variable's purposes to > > diverge. If info was overwritten with a distinct value, yet info_free > > still needed deallocation a bug would occur on this line. Preemptively > > address this issue (making use of divergent info/info_free values is > > under consideration). > > > > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> > > --- > > tools/xl/xl_info.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c > > index 3647468420..938f06f1a8 100644 > > --- a/tools/xl/xl_info.c > > +++ b/tools/xl/xl_info.c > > @@ -579,7 +579,7 @@ int main_list(int argc, char **argv) > > info, nb_domain); > > > > if (info_free) > > - libxl_dominfo_list_free(info, nb_domain); > > + libxl_dominfo_list_free(info_free, nb_domain); > > > > libxl_dominfo_dispose(&info_buf); > > > > I don't think this is the right thing to do with this patch. I disagree with this statement. > libxl_dominfo_list_free() should use the same variable that is used by > libxl_list_domain(). What we want to free is the allocation made by > libxl_list_domain(). I agree with this statement. > "info_free" in the function seems to be used as a boolean which tell > if "info" have been allocated or not. Actually, it probably say if > "info" is a list of "libxl_dominfo" or not. That may be what the author was thinking when they wrote lines 579 & 580. Problem is info_free is a pointer to libxl_dominfo, *not* a boolean. > So instead of just replacing "info" by "info_free" here, we should > instead store the result from libxl_list_domain() into a different > variable and free that, like it is done with "info_buf". > > I hope that makes sense? What you're describing seems to be precisely what the patch does. Perhaps you got the roles of "info" and "info_free" reversed? This actually points to an issue on lines 548 & 553. Instead of storing the return from libxl_list_domain() into "info" then copying to "info_free" both should be set at the same time. I had noticed this (and cringed), but didn't feel it was currently worthwhile to go after lines 548 & 553. If you want this additional change to accept the patch, I'm up for that.
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c index 3647468420..938f06f1a8 100644 --- a/tools/xl/xl_info.c +++ b/tools/xl/xl_info.c @@ -579,7 +579,7 @@ int main_list(int argc, char **argv) info, nb_domain); if (info_free) - libxl_dominfo_list_free(info, nb_domain); + libxl_dominfo_list_free(info_free, nb_domain); libxl_dominfo_dispose(&info_buf);
There is potential for the info and info_free variable's purposes to diverge. If info was overwritten with a distinct value, yet info_free still needed deallocation a bug would occur on this line. Preemptively address this issue (making use of divergent info/info_free values is under consideration). Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> --- tools/xl/xl_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)