Re: [Qemu-devel] [Qemu-block] [PATCH for-2.5?] qcow2: always initialize specific image info

[Denis V. Lunev]

On 12/07/2015 08:54 PM, Eric Blake wrote:
> On 12/07/2015 10:51 AM, Eric Blake wrote:
> > [adding qemu-devel - ALL patches should go to qemu-devel, even if they
> > are also going to a sub-list like qemu-block]
> >
> > On 12/07/2015 10:07 AM, Roman Kagan wrote:
> > > qcow2_get_specific_info() used to have a code path which would leave
> > > pointer to ImageInfoSpecificQCow2 uninitialized.
> > >
> > > We guess that it caused sporadic crashes on freeing an invalid pointer
> > > in response to "query-block" QMP command in
> > > visit_type_ImageInfoSpecificQCow2 with QapiDeallocVisitor.
> > >
> > > Although we have neither a solid proof nor a reproduction scenario,
> > > making sure the field is initialized appears a reasonable thing to do.
> > >
> > > Signed-off-by: Roman Kagan <address@hidden>
> > > ---
> > >   block/qcow2.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> Oops; hit send too soon.  I added for-2.5? to the subject line, because...
>
> > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > index 88f56c8..67c9d3d 100644
> > > --- a/block/qcow2.c
> > > +++ b/block/qcow2.c
> > > @@ -2739,7 +2739,7 @@ static ImageInfoSpecific 
> > > *qcow2_get_specific_info(BlockDriverState *bs)
> > >   
> > >       *spec_info = (ImageInfoSpecific){
> > >           .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
> > > -        .u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
> > > +        .u.qcow2 = g_new0(ImageInfoSpecificQCow2, 1),
> > NACK.  This makes no difference, except when s->qcow_version is out of spec.
> >
> > >       };
> > >       if (s->qcow_version == 2) {
> > >           *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){
> > If s->qcow_version is exactly 2, then we end up initializing all fields
> > due to the assignment here; same if qcow_version is exactly 3.  The only
> > time qcow2 remains uninitialized is if qcow_version is 0, 1, or > 3; but
> > we refuse to handle qcow files with out-of-range versions.  So I don't
> > see how you are plugging any uninitialized values; and therefore, I
> > don't see how this is patching any crashes.
> ...if you can prove that we aren't gracefully handling an out-of-spec
> qcow_version, such that the uninitialized memory in that scenario is
> indeed causing a crash, then it is worth respinning a v2 of this patch
> with that proof, and worth considering it for 2.5 if it really is a
> crash fixer.
Here is an info about our crash.

we have this crash under unknown conditions on RHEV version of QEMU.

Sorry, there is no much additional info. For the time being it
has happen only once.


*** Error in `/usr/libexec/qemu-kvm': free(): invalid pointer: 
0x00007f1c453757b8 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7d1fd)[0x7f1c450381fd]
/lib64/libglib-2.0.so.0(g_free+0xf)[0x7f1c49b5236f]
/usr/libexec/qemu-kvm(+0x1a71e9)[0x7f1c4ca0d1e9]
/usr/libexec/qemu-kvm(+0x1a779e)[0x7f1c4ca0d79e]
/usr/libexec/qemu-kvm(+0x1a7bf3)[0x7f1c4ca0dbf3]
/usr/libexec/qemu-kvm(+0x1a8664)[0x7f1c4ca0e664]
/usr/libexec/qemu-kvm(+0x1a92dd)[0x7f1c4ca0f2dd]
/usr/libexec/qemu-kvm(+0x1a9380)[0x7f1c4ca0f380]
/usr/libexec/qemu-kvm(+0x194eb8)[0x7f1c4c9faeb8]
/usr/libexec/qemu-kvm(+0xb35d8)[0x7f1c4c9195d8]
/usr/libexec/qemu-kvm(+0x301f02)[0x7f1c4cb67f02]
/usr/libexec/qemu-kvm(+0x31483f)[0x7f1c4cb7a83f]
/usr/libexec/qemu-kvm(+0x31490e)[0x7f1c4cb7a90e]
/usr/libexec/qemu-kvm(+0xb112f)[0x7f1c4c91712f]
/usr/libexec/qemu-kvm(+0x18a460)[0x7f1c4c9f0460]
/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x15a)[0x7f1c49b4c79a]
/usr/libexec/qemu-kvm(+0x2b96b8)[0x7f1c4cb1f6b8]
/usr/libexec/qemu-kvm(+0x87a4e)[0x7f1c4c8eda4e]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7f1c44fdcaf5]
/usr/libexec/qemu-kvm(+0x8c2bd)[0x7f1c4c8f22bd]
======= Memory map: ========

which we have decoded as

Decoded stacktrace:

g_free+0xf
visit_type_ImageInfoSpecificQCow2+169
visit_type_ImageInfoSpecific+302
visit_type_ImageInfo+867
visit_type_BlockDeviceInfo+820
visit_type_BlockInfo+685
visit_type_BlockInfoList+128
qmp_marshal_input_query_block+232
handle_qmp_command+1992
json_message_process_token+242
json_lexer_feed_char+383
json_lexer_feed+46
monitor_control_read+31
tcp_chr_read+144
g_main_context_dispatch+0x15a
main_loop_wait+440
main+5502
__libc_start_main+0xf5
_start+41

which looks like

More specifically (expanding inlines along the stack trace):

(gdb) l *(visit_type_ImageInfoSpecificQCow2+169)
0x1a71e9 is in visit_type_ImageInfoSpecificQCow2 (qapi-visit.c:552).
548     static void visit_type_ImageInfoSpecificQCow2_fields(Visitor *m, 
ImageInfoSpecificQCow2 **obj, Error **errp)
549     {
550         Error *err = NULL;
551 ==>     visit_type_str(m, &(*obj)->compat, "compat", &err);
552         if (err) {
553             goto out;
554         }


(gdb) l visit_type_str
238     void visit_type_str(Visitor *v, char **obj, const char *name, Error 
**errp)
239     {
240 ==>     v->type_str(v, obj, name, errp);
241     }


(gdb) l qapi_dealloc_visitor_new
175     QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
176     {
177         QapiDeallocVisitor *v;
178
179         v = g_malloc0(sizeof(*v));
[...]
191         v->visitor.type_str = qapi_dealloc_type_str;
192         v->visitor.type_number = qapi_dealloc_type_number;
193         v->visitor.type_size = qapi_dealloc_type_size;


(gdb) l qapi_dealloc_type_str
131     static void qapi_dealloc_type_str(Visitor *v, char **obj, const char 
*name,
132                                       Error **errp)
133     {
134         if (obj) {
135 ==>         g_free(*obj);
136         }
137     }

Den