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.