From: Elliott Mitchell <ehem+xen@m5p.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: xen-devel@lists.xenproject.org, Wei Liu <wl@xen.org>
Subject: Re: [PATCH 5/5] tools/xl: Fix potential deallocation bug
Date: Fri, 22 Apr 2022 15:58:00 -0700 [thread overview]
Message-ID: <YmMy+LaG1RqXMrlb@mattapan.m5p.com> (raw)
In-Reply-To: <YdWzswCszMN1IE20@perard>
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.
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | ehem+sigmsg@m5p.com PGP 87145445 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
next prev parent reply other threads:[~2022-04-22 22:59 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-27 7:39 [PATCH 0/5] Some misc from my tree Elliott Mitchell
2020-12-10 23:09 ` [PATCH 5/5] tools/xl: Fix potential deallocation bug Elliott Mitchell
2021-12-29 17:20 ` Luca Fancellu
2022-01-05 15:05 ` Anthony PERARD
2022-04-22 22:58 ` Elliott Mitchell [this message]
2020-12-18 1:42 ` [PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...() Elliott Mitchell
2021-12-29 17:18 ` Luca Fancellu
2020-12-18 1:42 ` [PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain() Elliott Mitchell
2021-12-29 17:19 ` Luca Fancellu
2020-12-18 21:32 ` [PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const Elliott Mitchell
2021-12-29 17:18 ` Luca Fancellu
2020-12-18 21:37 ` [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant Elliott Mitchell
2021-12-29 17:15 ` Luca Fancellu
2022-01-05 10:09 ` Anthony PERARD
2022-01-05 13:09 ` Luca Fancellu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YmMy+LaG1RqXMrlb@mattapan.m5p.com \
--to=ehem+xen@m5p.com \
--cc=anthony.perard@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.