[libvirt] [PATCH 2/7] conf: prevent crash with no uuid in cephx auth secret

[Ján Tomko]

On 11/28/12 15:31, Osier Yang wrote:
> On 2012年11月28日 21:34, Ján Tomko wrote:
>> Also remove the pointles check for NULL in auth.cephx.secret.uuid,
>> since this is a static array.
> 
> It's nice if there is log of coverity.

Error: FORWARD_NULL (CWE-476):
libvirt-0.10.2/src/conf/storage_conf.c:447: cond_false: Condition
"auth->username == NULL", taking false branch
libvirt-0.10.2/src/conf/storage_conf.c:451: if_end: End of if statement
libvirt-0.10.2/src/conf/storage_conf.c:455: cond_true: Condition "uuid
== NULL", taking true branch
libvirt-0.10.2/src/conf/storage_conf.c:455: var_compare_op: Comparing
"uuid" to null implies that "uuid" might be null.
libvirt-0.10.2/src/conf/storage_conf.c:455: cond_false: Condition
"auth->secret.usage == NULL", taking false branch
libvirt-0.10.2/src/conf/storage_conf.c:459: if_end: End of if statement
libvirt-0.10.2/src/conf/storage_conf.c:461: var_deref_model: Passing
null pointer "uuid" to function "virUUIDParse(char const *, unsigned
char *)", which dereferences it. (The dereference is assumed on the
basis of the 'nonnull' parameter attribute.)

Error: NO_EFFECT (CWE-398):
libvirt-0.10.2/src/conf/storage_conf.c:979: array_null: Comparing an
array to null is not useful: "src->auth.cephx.secret.uuid != NULL".

>> ...
> 
> This change forces both of "uuid" and "usage" to be specified.
> 
> But from the RNG schema:
> 
>   <define name='sourceinfoauthsecret'>
>     <element name='secret'>
>       <choice>
>         <attribute name='uuid'>
>           <text/>
>         </attribute>
>         <attribute name='usage'>
>           <text/>
>         </attribute>
>       </choice>
>     </element>
>   </define>
> 
> Means that it allows only one of them specified.
> 
> Hm, from the schema, it should error out if both of them are
> specified too. So either there is problem of either the schema
> or the codes.
> 
> I think we have to figure out if the schema is correct first.

Looking at the code in storage_backend_rbd.c it looks like if both are
specified, only usage is taken into account:

if (pool->def->source.auth.cephx.secret.uuid != NULL) {
    virUUIDFormat(pool->def->source.auth.cephx.secret.uuid, secretUuid);
    VIR_DEBUG("Looking up secret by UUID: %s", secretUuid);
    secret = virSecretLookupByUUIDString(conn, secretUuid);
}

if (pool->def->source.auth.cephx.secret.usage != NULL) {
    VIR_DEBUG("Looking up secret by usage: %s",
              pool->def->source.auth.cephx.secret.usage);
    secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH,
                          pool->def->source.auth.cephx.secret.usage);
}

I'll send another version shortly.

Jan