"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> * Daniel P. Berrangé (berra...@redhat.com) wrote:
>> On Fri, Mar 20, 2020 at 05:31:17PM +0000, Dr. David Alan Gilbert wrote:
>> > (Rearranging the text a bit)
>> >
>> > * Markus Armbruster (arm...@redhat.com) wrote:
>> >
>> > > David (cc'ed) should be able to tell us which fix is right.
>> > >
>> > > @tls_creds and @tls_hostname look like they could have the same issue.
>> >
>> > A certain Markus removed the Null checks in 8cc99dc because 4af245d
>> > guaranteed they would be None-Null for tls-creds/hostname - so we
>> > should be OK for those.
>> >
>> > But tls-authz came along a lot later in d2f1d29 and doesn't
>> > seem to have the initialisation, which is now in
>> > migration_instance_init.
>> >
>> > So I *think* the fix for this is to do the modern equivalent of 4af245d
>> > :
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index c1d88ace7f..0bc1b93277 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -3686,6 +3686,7 @@ static void migration_instance_init(Object *obj)
>> >
>> > params->tls_hostname = g_strdup("");
>> > params->tls_creds = g_strdup("");
>> > + params->tls_authz = g_strdup("");
>> >
>> > /* Set has_* up only for parameter checks */
>> > params->has_compress_level = true;
>> >
>> > Copying in Dan to check that wouldn't break tls.
>>
>> It *will* break TLS, because it will cause the TLS code to lookup
>> an object with the ID of "". NULL must be preserved when calling
>> the TLS APIs.
>
> OK, good I asked...
>
>> The assignment of "" to tls_hostname would also have broken TLS,
>> so the migration_tls_channel_connect method had to turn it back
>> into a real NULL.
>>
>> The use of "" for tls_creds will similarly cause it to try and
>> lookup an object with ID of "", and fail. That one's harmless
>> though, because it would also fail if it were NULL.
>
> OK.
>
> It looks like the output of query-migrate-parameters though already
> turns it into "", so I don't think you can tell it's NULL from that:
>
> {"QMP": {"version": {"qemu": {"micro": 0, "minor": 2, "major": 4}, "package":
> "qemu-4.2.0-4.fc31"}, "capabilities": ["oob"]}}
> { "execute": "qmp_capabilities" }
> {"return": {}}
> { "execute": "query-migrate-parameters" }
> {"return": {"xbzrle-cache-size": 67108864, "cpu-throttle-initial": 20,
> "announce-max": 550, "decompress-threads": 2, "compress-threads": 8,
> "compress-level": 1, "multifd-channels": 2, "announce-initial": 50,
> "block-incremental": false, "compress-wait-thread": true, "downtime-limit":
> 300, "tls-authz": "", "announce-rounds": 5, "announce-step": 100,
> "tls-creds": "", "max-cpu-throttle": 99, "max-postcopy-bandwidth": 0,
> "tls-hostname": "", "max-bandwidth": 33554432, "x-checkpoint-delay": 20000,
> "cpu-throttle-increment": 10}}
>
> I'm not sure who turns a Null into a "" but I guess it's somewhere in
> the json output iterator.
It's this wart:
static void qobject_output_type_str(Visitor *v, const char *name, char
**obj,
Error **errp)
{
QObjectOutputVisitor *qov = to_qov(v);
if (*obj) {
qobject_output_add(qov, name, qstring_from_str(*obj));
} else {
qobject_output_add(qov, name, qstring_from_str(""));
}
}
Warty since day 1.
In theory, !*obj should not happen, since QAPI type 'str' is not
nullable.
In practice, it handwritten code can easily make it happen.
> So we can fix this problem either in qmp_query_migrate_parameters
> and just strdup a "", or substitute it in hmp_info_migrate_parameters.
Fixing it in qmp_query_migrate_parameters() is cleaner: it avoids null
'str', and bypasses the wart.
