[libvirt] [PATCH] x86Decode: avoid NULL-dereference upon questionable input

[Jim Meyering]

Eric Blake wrote:

> According to Jim Meyering on 3/1/2010 1:21 PM:
>> Clang warned about the potential NULL-dereference
>> via the STREQ/strcmp below.  models[i] could be NULL.
>> Even "models" could be NULL, and the "allowed = ..." test
>> makes that appear to be deliberately allowed.
>
> This same function was also listed by coverity, but only for models, not
> models[i].

Yes, I was disappointed to see Coverity missed that.

>> The change below is the least invasive and cleanest
>> I could come up with, assuming there is no need to diagnose
>> (e.g., by returning -1) the condition of a NULL models[i] pointer.
>>
>>      while (candidate != NULL) {
>>          bool allowed = (models == NULL);
>>
>>          for (i = 0; i < candidate->ncpuid; i++) {
>>              cpuid = x86DataCpuid(data, candidate->cpuid[i].function);
>>              if (cpuid == NULL
>>                  || !x86cpuidMatchMasked(cpuid, candidate->cpuid + i))
>>                  goto next;
>>          }
>>
>>          for (i = 0; i < nmodels; i++) {
>> -            if (STREQ(models[i], candidate->name)) {
>> +            if (models && models[i] && STREQ(models[i], candidate->name)) {
>
> Isn't the intent that (models==NULL) iff (nmodels==0)?

That is the intent, but the code at this level does not detect the mismatch.
I think someone made a change recently to protect us at a higher
(cpu-independent) level.
But that doesn't help us here, if a new caller of this function
violates those higher-level constraints.

> In which case,
> this code is unreachable if models is NULL.  But your patch certainly is
> the least-invasive possible, and while it is only a false positive for
> well-formed arguments, I didn't spend time checking all clients of
> x86Decode to see if there is ever a possibility of bad arguments.
>
> ACK

Thanks.