Potential NULL dereference in hyperv_find_vcpu() before checking result of qemu_get_cpu()
Hello! We were running static analysis on Qemu with Svace and found 2 instances of potentially unreachable code. During static analysis of QEMU, a potential NULL pointer dereference was found in hyperv_find_vcpu(). Marker: https://svacer.community.ispras.ru/mode/review/project/197eff20-87d9-4daf-b5e4-311954e2d57f/branch/d957f339-3396-4972-9015-72073dc5ec73/snapshot/7c48b0ac-b6d8-496e-952f-f05cf637f143/marker/eyJtYXJrZXJJRCI6ICI3MjFkYzNjMS0zMDYwLTQwYmUtYjNmMS0zM2Q2NjNlOWZmNjkiLCAiZmlsZSI6ICIvaHcvaHlwZXJ2L2h5cGVydi5jIn0= Pointer 'cs', returned from function 'qemu_get_cpu' at hyperv.c:229, may be NULL and is passed as 1st parameter in call to function 'hyperv_vp_index' at hyperv.c:230, where it is dereferenced at hyperv.h:75. Problematic code: static CPUState \*hyperv_find_vcpu(uint32_t vp_index) {     CPUState \*cs = qemu_get_cpu(vp_index);     assert(hyperv_vp_index(cs) == vp_index);     return cs; } qemu_get_cpu() can return NULL if no CPU with the requested index is found: CPUState \*qemu_get_cpu(int index) {     CPUState \*cpu;       CPU_FOREACH(cpu) {         if (cpu-\>cpu_index == index) {             return cpu;         }     }       return NULL; } However, the returned pointer is passed to hyperv_vp_index() before any NULL check: static inline uint32_t hyperv_vp_index(CPUState \*cs) {     return cs-\>cpu_index; } As a result, if qemu_get_cpu(vp_index) returns NULL, hyperv_vp_index(cs) will dereference a NULL pointer. In the current code, the result is checked later in the caller (hyperv_sint_route_new()), but this check happens after hyperv_find_vcpu() has already used cs inside the assertion. Therefore, the validation is placed too late and does not protect the dereference inside hyperv_find_vcpu(). It would be better to move the NULL check into hyperv_find_vcpu() before calling hyperv_vp_index(), for example: static CPUState \*hyperv_find_vcpu(uint32_t vp_index) {     CPUState \*cs = qemu_get_cpu(vp_index);       if (cs == NULL) {         return NULL;     }       assert(hyperv_vp_index(cs) == vp_index);     return cs; } Then the caller can continue to handle the NULL result as it already does. This makes the helper function safe by itself and removes the potential NULL dereference reported by the analyzer.
issue