* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.5] qcow2: always initialize specific image info [not found] <1449508029-14664-1-git-send-email-rkagan@virtuozzo.com> @ 2015-12-07 17:51 ` Eric Blake 2015-12-07 17:54 ` [Qemu-devel] [Qemu-block] [PATCH for-2.5?] " Eric Blake 0 siblings, 1 reply; 5+ messages in thread From: Eric Blake @ 2015-12-07 17:51 UTC (permalink / raw) To: Roman Kagan, Kevin Wolf, qemu-block; +Cc: Denis Lunev, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1922 bytes --] [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 <rkagan@virtuozzo.com> > --- > block/qcow2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.5?] qcow2: always initialize specific image info 2015-12-07 17:51 ` [Qemu-devel] [Qemu-block] [PATCH for-2.5] qcow2: always initialize specific image info Eric Blake @ 2015-12-07 17:54 ` Eric Blake 2015-12-07 18:11 ` Denis V. Lunev 0 siblings, 1 reply; 5+ messages in thread From: Eric Blake @ 2015-12-07 17:54 UTC (permalink / raw) To: Roman Kagan, Kevin Wolf, qemu-block; +Cc: Denis Lunev, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2399 bytes --] 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 <rkagan@virtuozzo.com> >> --- >> 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.5?] qcow2: always initialize specific image info 2015-12-07 17:54 ` [Qemu-devel] [Qemu-block] [PATCH for-2.5?] " Eric Blake @ 2015-12-07 18:11 ` Denis V. Lunev 2015-12-07 19:44 ` Max Reitz 0 siblings, 1 reply; 5+ messages in thread From: Denis V. Lunev @ 2015-12-07 18:11 UTC (permalink / raw) To: Eric Blake, Roman Kagan, Kevin Wolf, qemu-block; +Cc: qemu-devel 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 <rkagan@virtuozzo.com> >>> --- >>> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.5?] qcow2: always initialize specific image info 2015-12-07 18:11 ` Denis V. Lunev @ 2015-12-07 19:44 ` Max Reitz 2015-12-08 9:37 ` Kevin Wolf 0 siblings, 1 reply; 5+ messages in thread From: Max Reitz @ 2015-12-07 19:44 UTC (permalink / raw) To: Denis V. Lunev, Eric Blake, Roman Kagan, Kevin Wolf, qemu-block Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 6123 bytes --] On 07.12.2015 19:11, Denis V. Lunev wrote: > 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 <rkagan@virtuozzo.com> >>>> --- >>>> 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. More or less unfortunately, Eric is right. I chose a g_new() over g_new0() because I was using compound literals anyway (and members not explicitly initialized in the initializer list given for a compound literal are initialized implicitly like object with static storage duration, i.e. 0, generally). s->qcow_version is always set to 2 or 3. It is only set in: (1) qcow2_open(): Directly before it is set, we check that the version is either 2 or 3. (2) qcow2_downgrade(): We make sure that target_version (the value s->qcow_version is set to) is equal to 2. (3) qcow2_downgrade(): On error, s->qcow_version may be reset to current_version, which is the old value of s->qcow_version which we can inductively assume to be either 2 or 3. (4) qcow2_amend_options(): On version upgrade, s->qcow_version is overwritten by new_version, which is either 2, 3, or the old value of s->qcow_version (in practice it is always 3 because it needs to be greater than s->qcow_version in order to get here). (5) qcow2_amend_options(): On version upgrade error, s->qcow_version is reset to the old value of s->qcow_version, which we can again assume to be 2 or 3. I'd be completely fine with adding an "else { abort(); }" branch to qcow2_get_specific_info(). But I fear that the issue you encountered is caused by something different than the ImageInfoSpecificQCow2 object not being fully initialized, and therefore I'm against this patch even if it should not change anything (because it might make as feel as if we found the issue even though there (most probably) is none here). Also... > 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 [...] > 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); ...the compat field is always set, in either code path (both for version 2 and 3). Therefore, if this pointer is wrong, it definitely is not due to the field not being initialized. Without any way of reproducing, it is difficult to find the true cause of the issue, though. valgrind would probably tell us something, but for that we'd need something it could find first. Max > 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 > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.5?] qcow2: always initialize specific image info 2015-12-07 19:44 ` Max Reitz @ 2015-12-08 9:37 ` Kevin Wolf 0 siblings, 0 replies; 5+ messages in thread From: Kevin Wolf @ 2015-12-08 9:37 UTC (permalink / raw) To: Max Reitz; +Cc: Denis V. Lunev, Roman Kagan, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 295 bytes --] Am 07.12.2015 um 20:44 hat Max Reitz geschrieben: > I'd be completely fine with adding an "else { abort(); }" branch to > qcow2_get_specific_info(). This is actually what I was going to suggest, too. Of course, it's not supposed to fix anything now, but defensive coding has never hurt. Kevin [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-08 9:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1449508029-14664-1-git-send-email-rkagan@virtuozzo.com> 2015-12-07 17:51 ` [Qemu-devel] [Qemu-block] [PATCH for-2.5] qcow2: always initialize specific image info Eric Blake 2015-12-07 17:54 ` [Qemu-devel] [Qemu-block] [PATCH for-2.5?] " Eric Blake 2015-12-07 18:11 ` Denis V. Lunev 2015-12-07 19:44 ` Max Reitz 2015-12-08 9:37 ` Kevin Wolf
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.