Re: [Qemu-devel] [PATCH 2/4] monitor: fix access freed memory

[zhanghailiang]

Hi Alex,
Thanks for your quick reply.
On 2014/8/4 17:01, Alex Bennée wrote:
> zhanghailiang writes:
>
> > The function monitor_fdset_dup_fd_find_remove() references member of 'mon_fdset'
> > which may be freed in function monitor_fdset_cleanup()
> >
> > Signed-off-by: zhanghailiang<address@hidden>
> > ---
> >   monitor.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 5bc70a6..41e46a6 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2532,8 +2532,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, 
> > bool remove)
> >   {
> >       MonFdset *mon_fdset;
> >       MonFdsetFd *mon_fdset_fd_dup;
> > +    int64_t id = -1;
> >
> >       QLIST_FOREACH(mon_fdset,&mon_fdsets, next) {
> > +        id = mon_fdset->id;
> >           QLIST_FOREACH(mon_fdset_fd_dup,&mon_fdset->dup_fds, next) {
> >               if (mon_fdset_fd_dup->fd == dup_fd) {
> >                   if (remove) {
> > @@ -2542,7 +2544,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, 
> > bool remove)
> >                           monitor_fdset_cleanup(mon_fdset);
> >                       }
> >                   }
> > -                return mon_fdset->id;
> > +                return id;
> >               }
> >           }
> >       }
>
> If monitor_fdset_cleanup closes the FD won't you now be passing an
                                                          ^^^^^^^^^^
> invalid fd to the calling function?
^^^^^^^^^^^^^
Sorry for my bad english, do you mean return an invalid id to the caller?
I think there is no problem, and the calling process is:
qemu_close()
(1)|--->fdset_id = monitor_fdset_dup_fd_find
       |--->monitor_fdset_dup_fd_find_remove(dup_fd, false)
           |--->monitor_fdset_dup_fd_find_remove
               |--->>>remove=false, no problem to return id or -1.
(2)|--->(ingore return value)monitor_fdset_dup_fd_remove
       |--->monitor_fdset_dup_fd_find_remove(dup_fd, true)
           |--->monitor_fdset_dup_fd_find_remove
               |--->>>remove=true, mon_fdset may be freed, and fault to 
return mon_fdset->id
As you see above, monitor_fdset_dup_fd_find_remove() has two 
capabilities (find/remove) which distinguished by 'bool remove'. It may 
free the mon_fdset only when remove=true, as show step(2), return value 
id will be ignored, so it is no problem to return an invaild id in this 
case.
Here we just solve the bug of accessing freed memory but not change its 
original logic.

Thanks,
zhanghailiang