[Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data

[Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data

[fangying]

Hi Gerd,

Memory leak is observed in zrle_compress_data when we are doing some 
AddressSanitizer tests. The leak stack is as bellow:

=================================================================
==47887==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 29760 byte(s) in 5 object(s) allocated from:
     #0 0xffffa67ef3c3 in __interceptor_calloc (/lib64/libasan.so.4+0xd33c3)
     #1 0xffffa65071cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
     #2 0xffffa5e968f7 in deflateInit2_ (/lib64/libz.so.1+0x78f7)
     #3 0xaaaacec58613 in zrle_compress_data ui/vnc-enc-zrle.c:87
     #4 0xaaaacec58613 in zrle_send_framebuffer_update ui/vnc-enc-zrle.c:344
     #5 0xaaaacec34e77 in vnc_send_framebuffer_update ui/vnc.c:919
     #6 0xaaaacec5e023 in vnc_worker_thread_loop ui/vnc-jobs.c:271
     #7 0xaaaacec5e5e7 in vnc_worker_thread ui/vnc-jobs.c:340
     #8 0xaaaacee4d3c3 in qemu_thread_start util/qemu-thread-posix.c:502
     #9 0xffffa544e8bb in start_thread (/lib64/libpthread.so.0+0x78bb)
     #10 0xffffa53965cb in thread_start (/lib64/libc.so.6+0xd55cb)

This leak stack can be easily reproduced in the case that a client repeatedly 
does vnc connect/disconnect .

To get the leak stack, we can compile qemu with
--extra-ldflags=-Wl,--build-id -Wl,-z,relro -Wl,-z,now -lasan'
'--extra-cflags=-O0 -g -fno-omit-frame-pointer -fno-stack-protector 
-fsanitize=address -fsanitize=leak' using gcc that supports asan.

It is obvious that there may be memory leak in the zlib/zrle compress stuff.
IIUC, *deflateInit2* is called with the local VncState vs->zrle.stream when a 
client is connected the vnc server. And then *deflate* is called to do the 
encoding. Finally *deflateEnd* is called in vnc_zrle_clear when a connection is
closed.

I had not think it out why this memory leak could happen here.
It is noticed that deflateInit2 is called with the local vs,
however deflateEnd is called with the origin vs.
The local vs is copied to the origin vs in vnc_async_encoding_start and 
vnc_async_encoding_end on the contrary.

Have you got any idea on this issue ?

Thanks.
Ying Fang

Re: [Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data

[Li Qiang]

fangying <fangying1@huawei.com> 于2019年8月31日周六 上午8:45写道：

> Hi Gerd,
>
> Memory leak is observed in zrle_compress_data when we are doing some
> AddressSanitizer tests. The leak stack is as bellow:
>
> =================================================================
> ==47887==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 29760 byte(s) in 5 object(s) allocated from:
>      #0 0xffffa67ef3c3 in __interceptor_calloc
> (/lib64/libasan.so.4+0xd33c3)
>      #1 0xffffa65071cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>      #2 0xffffa5e968f7 in deflateInit2_ (/lib64/libz.so.1+0x78f7)
>      #3 0xaaaacec58613 in zrle_compress_data ui/vnc-enc-zrle.c:87
>      #4 0xaaaacec58613 in zrle_send_framebuffer_update
> ui/vnc-enc-zrle.c:344
>      #5 0xaaaacec34e77 in vnc_send_framebuffer_update ui/vnc.c:919
>      #6 0xaaaacec5e023 in vnc_worker_thread_loop ui/vnc-jobs.c:271
>      #7 0xaaaacec5e5e7 in vnc_worker_thread ui/vnc-jobs.c:340
>      #8 0xaaaacee4d3c3 in qemu_thread_start util/qemu-thread-posix.c:502
>      #9 0xffffa544e8bb in start_thread (/lib64/libpthread.so.0+0x78bb)
>      #10 0xffffa53965cb in thread_start (/lib64/libc.so.6+0xd55cb)
>
> This leak stack can be easily reproduced in the case that a client
> repeatedly
> does vnc connect/disconnect .
>
> To get the leak stack, we can compile qemu with
> --extra-ldflags=-Wl,--build-id -Wl,-z,relro -Wl,-z,now -lasan'
> '--extra-cflags=-O0 -g -fno-omit-frame-pointer -fno-stack-protector
> -fsanitize=address -fsanitize=leak' using gcc that supports asan.
>
> It is obvious that there may be memory leak in the zlib/zrle compress
> stuff.
> IIUC, *deflateInit2* is called with the local VncState vs->zrle.stream
> when a
> client is connected the vnc server. And then *deflate* is called to do the
> encoding. Finally *deflateEnd* is called in vnc_zrle_clear when a
> connection is
> closed.
>
> I had not think it out why this memory leak could happen here.
> It is noticed that deflateInit2 is called with the local vs,
> however deflateEnd is called with the origin vs.
> The local vs is copied to the origin vs in vnc_async_encoding_start and
> vnc_async_encoding_end on the contrary.
>
> Have you got any idea on this issue ?
>
>
Hello Ying,

I have posted a patch for this issue:
--> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg06675.html

Please check whether the patch can address this issue.

Thanks,
Li Qiang





> Thanks.
> Ying Fang
>
>
>

Re: [Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data

[fangying]

Nice work, your patch does fix this issue in my test.

I think we should make VncState.zlib to be a pointer type as well.

Since we are going to use pointers instead of copy, we must make sure that there’s no race condition of pointer members between the local vs (vnc worker thread) and origin vs (main thread).


Thanks.
Ying Fang
发件人： Li Qiang<liq3ea@gmail.com<mailto:liq3ea@gmail.com>>
收件人： fangying<fangying1@huawei.com<mailto:fangying1@huawei.com>>
抄送： Gerd Hoffmann<kraxel@redhat.com<mailto:kraxel@redhat.com>>;qemu-devel<qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org>>;Daniel P. Berrange<berrange@redhat.com<mailto:berrange@redhat.com>>;zhouyibo<zhouyibo3@huawei.com<mailto:zhouyibo3@huawei.com>>
主题： Re: [Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data
时间： 2019-08-31 23:48:10



fangying <fangying1@huawei.com<mailto:fangying1@huawei.com>> 于2019年8月31日周六 上午8:45写道：
Hi Gerd,

Memory leak is observed in zrle_compress_data when we are doing some
AddressSanitizer tests. The leak stack is as bellow:

=================================================================
==47887==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 29760 byte(s) in 5 object(s) allocated from:
     #0 0xffffa67ef3c3 in __interceptor_calloc (/lib64/libasan.so.4+0xd33c3)
     #1 0xffffa65071cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
     #2 0xffffa5e968f7 in deflateInit2_ (/lib64/libz.so.1+0x78f7)
     #3 0xaaaacec58613 in zrle_compress_data ui/vnc-enc-zrle.c:87
     #4 0xaaaacec58613 in zrle_send_framebuffer_update ui/vnc-enc-zrle.c:344
     #5 0xaaaacec34e77 in vnc_send_framebuffer_update ui/vnc.c:919
     #6 0xaaaacec5e023 in vnc_worker_thread_loop ui/vnc-jobs.c:271
     #7 0xaaaacec5e5e7 in vnc_worker_thread ui/vnc-jobs.c:340
     #8 0xaaaacee4d3c3 in qemu_thread_start util/qemu-thread-posix.c:502
     #9 0xffffa544e8bb in start_thread (/lib64/libpthread.so.0+0x78bb)
     #10 0xffffa53965cb in thread_start (/lib64/libc.so.6+0xd55cb)

This leak stack can be easily reproduced in the case that a client repeatedly
does vnc connect/disconnect .

To get the leak stack, we can compile qemu with
--extra-ldflags=-Wl,--build-id -Wl,-z,relro -Wl,-z,now -lasan'
'--extra-cflags=-O0 -g -fno-omit-frame-pointer -fno-stack-protector
-fsanitize=address -fsanitize=leak' using gcc that supports asan.

It is obvious that there may be memory leak in the zlib/zrle compress stuff.
IIUC, *deflateInit2* is called with the local VncState vs->zrle.stream when a
client is connected the vnc server. And then *deflate* is called to do the
encoding. Finally *deflateEnd* is called in vnc_zrle_clear when a connection is
closed.

I had not think it out why this memory leak could happen here.
It is noticed that deflateInit2 is called with the local vs,
however deflateEnd is called with the origin vs.
The local vs is copied to the origin vs in vnc_async_encoding_start and
vnc_async_encoding_end on the contrary.

Have you got any idea on this issue ?


Hello Ying,

I have posted a patch for this issue:
--> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg06675.html

Please check whether the patch can address this issue.

Thanks,
Li Qiang




Thanks.
Ying Fang

Re: [Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data

[Li Qiang]

fangying <fangying1@huawei.com> 于2019年9月1日周日 下午8:29写道：

> Nice work, your patch does fix this issue in my test.
>
> I think we should make VncState.zlib to be a pointer type as well.
>
> Since we are going to use pointers instead of copy, we must make sure that
> there’s no race condition of pointer members between the local vs (vnc
> worker thread) and origin vs (main thread).
>
>

Yes, there is a race between the main thread and vnc thread. Maybe we
should add a lock just like the 'vs->output_mutex'.
Let's wait for Gerd's (or others) comments whether there is a better way
than using pointer before sending the revision.

Thanks,
Li Qiang


>
> Thanks.
> Ying Fang
> *发件人： *Li Qiang<liq3ea@gmail.com>
> *收件人： *fangying<fangying1@huawei.com>
> *抄送： *Gerd Hoffmann<kraxel@redhat.com>;qemu-devel<qemu-devel@nongnu.org>;Daniel
> P. Berrange<berrange@redhat.com>;zhouyibo<zhouyibo3@huawei.com>
> *主题： *Re: [Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data
> *时间： *2019-08-31 23:48:10
>
>
>
> fangying <fangying1@huawei.com> 于2019年8月31日周六 上午8:45写道：
>
>> Hi Gerd,
>>
>> Memory leak is observed in zrle_compress_data when we are doing some
>> AddressSanitizer tests. The leak stack is as bellow:
>>
>> =================================================================
>> ==47887==ERROR: LeakSanitizer: detected memory leaks
>>
>> Direct leak of 29760 byte(s) in 5 object(s) allocated from:
>>      #0 0xffffa67ef3c3 in __interceptor_calloc
>> (/lib64/libasan.so.4+0xd33c3)
>>      #1 0xffffa65071cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>>      #2 0xffffa5e968f7 in deflateInit2_ (/lib64/libz.so.1+0x78f7)
>>      #3 0xaaaacec58613 in zrle_compress_data ui/vnc-enc-zrle.c:87
>>      #4 0xaaaacec58613 in zrle_send_framebuffer_update
>> ui/vnc-enc-zrle.c:344
>>      #5 0xaaaacec34e77 in vnc_send_framebuffer_update ui/vnc.c:919
>>      #6 0xaaaacec5e023 in vnc_worker_thread_loop ui/vnc-jobs.c:271
>>      #7 0xaaaacec5e5e7 in vnc_worker_thread ui/vnc-jobs.c:340
>>      #8 0xaaaacee4d3c3 in qemu_thread_start util/qemu-thread-posix.c:502
>>      #9 0xffffa544e8bb in start_thread (/lib64/libpthread.so.0+0x78bb)
>>      #10 0xffffa53965cb in thread_start (/lib64/libc.so.6+0xd55cb)
>>
>> This leak stack can be easily reproduced in the case that a client
>> repeatedly
>> does vnc connect/disconnect .
>>
>> To get the leak stack, we can compile qemu with
>> --extra-ldflags=-Wl,--build-id -Wl,-z,relro -Wl,-z,now -lasan'
>> '--extra-cflags=-O0 -g -fno-omit-frame-pointer -fno-stack-protector
>> -fsanitize=address -fsanitize=leak' using gcc that supports asan.
>>
>> It is obvious that there may be memory leak in the zlib/zrle compress
>> stuff.
>> IIUC, *deflateInit2* is called with the local VncState vs->zrle.stream
>> when a
>> client is connected the vnc server. And then *deflate* is called to do
>> the
>> encoding. Finally *deflateEnd* is called in vnc_zrle_clear when a
>> connection is
>> closed.
>>
>> I had not think it out why this memory leak could happen here.
>> It is noticed that deflateInit2 is called with the local vs,
>> however deflateEnd is called with the origin vs.
>> The local vs is copied to the origin vs in vnc_async_encoding_start and
>> vnc_async_encoding_end on the contrary.
>>
>> Have you got any idea on this issue ?
>>
>>
> Hello Ying,
>
> I have posted a patch for this issue:
> --> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg06675.html
>
> Please check whether the patch can address this issue.
>
> Thanks,
> Li Qiang
>
>
>
>
>
>> Thanks.
>> Ying Fang
>>
>>
>>

Re: [Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data

[Gerd Hoffmann]

On Sun, Sep 01, 2019 at 10:43:42PM +0800, Li Qiang wrote:
> fangying <fangying1@huawei.com> 于2019年9月1日周日 下午8:29写道：
> 
> > Nice work, your patch does fix this issue in my test.
> >
> > I think we should make VncState.zlib to be a pointer type as well.
> >
> > Since we are going to use pointers instead of copy, we must make sure that
> > there’s no race condition of pointer members between the local vs (vnc
> > worker thread) and origin vs (main thread).
> >
> >
> 
> Yes, there is a race between the main thread and vnc thread.

Where do you see a race?  The main thread allocates the data structures
before any job is started, cleans up after the jobs have been stopped
and never accesses them otherwise.

So unless I missed something the data structures are never accessed in
parallel from multiple threads.

> Maybe we should add a lock just like the 'vs->output_mutex'.

output is a different story.  The output buffer is accessed in parallel
(job thread produces and main thread consumes), so we actually need a
lock here for synchronization.

cheers,
  Gerd

Re: [Qemu-devel] Discussion: vnc: memory leak in zrle_compress_data

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2019年9月19日周四 下午4:54写道：

> On Sun, Sep 01, 2019 at 10:43:42PM +0800, Li Qiang wrote:
> > fangying <fangying1@huawei.com> 于2019年9月1日周日 下午8:29写道：
> >
> > > Nice work, your patch does fix this issue in my test.
> > >
> > > I think we should make VncState.zlib to be a pointer type as well.
> > >
> > > Since we are going to use pointers instead of copy, we must make sure
> that
> > > there’s no race condition of pointer members between the local vs (vnc
> > > worker thread) and origin vs (main thread).
> > >
> > >
> >
> > Yes, there is a race between the main thread and vnc thread.
>
> Where do you see a race?  The main thread allocates the data structures
> before any job is started, cleans up after the jobs have been stopped
> and never accesses them otherwise.
>
> So unless I missed something the data structures are never accessed in
> parallel from multiple threads.
>

I checked the code again. Seems you're right.

Thanks Gerd, this strengthen my understanding of the vnc working mechanism.


Thanks,
Li Qiang


>
> > Maybe we should add a lock just like the 'vs->output_mutex'.
>
> output is a different story.  The output buffer is accessed in parallel
> (job thread produces and main thread consumes), so we actually need a
> lock here for synchronization.
>
> cheers,
>   Gerd
>
>