{"id":405512,"date":"2024-06-29T18:22:07","date_gmt":"2024-06-29T18:22:07","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=405512"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=405512","title":{"rendered":"Checking QEMU using PVS-Studio<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n
\"image1.png\"<\/div>\n

QEMU is a rather well-known application for emulation. Static analysis can help developers of complex projects such as QEMU catch errors at early stages and generally improve quality and reliability of a project. In this article, we will check the source code of the QEMU application for potential vulnerabilities and errors using the PVS-Studio static analysis tool.
<\/a>
QEMU is free software designed to emulate the hardware of various platforms. It allows you to run applications and operating systems on hardware platforms that are different from target ones. For example, it is possible to run an application written for MIPS for the x86 architecture. QEMU also supports emulation of various peripherals, such as video cards, usb, etc. The project is quite complex and worthy of attention. Such projects are of interest in terms of static analysis, so we decided to scan its code using PVS-Studio.<\/p>\n

About the analysis<\/h2>\n

The source code of the project can be obtained from the mirror on github<\/a>. The project is quite large and can be compiled for various platforms. For easier code checking, let’s use the PVS-Studio compilation monitoring system<\/a>. This system is designed for very simple integration of static analysis into almost any build platform. The system is based on tracking compiler calls during the build and allows you to collect all the information for later files analysis. In other words, we just run the build, PVS-Studio collects the necessary information, and then we run the analysis \u2014 everything is simple. Details can be found by the link above.<\/p>\n

After checking, the analyzer found a lot of potential problems. As for diagnostics related to general analysis, we got: 1940 diagnostics of High level, 1996 \u2014 Medium level, 9596 \u2014 Low level. After viewing all the warnings, I decided to focus on the diagnostics of the High level of certainty. There were quite a few such warnings (1940), but most of them are either of the same type or are associated with repeated use of a suspicious macro. For example, let’s take a look at the g_new<\/i> macro.<\/p>\n

#define g_new(struct_type, n_structs)                         _G_NEW (struct_type, n_structs, malloc)  #define _G_NEW(struct_type, n_structs, func)       \\   (struct_type *) (G_GNUC_EXTENSION ({             \\     gsize __n = (gsize) (n_structs);               \\     gsize __s = sizeof (struct_type);              \\     gpointer __p;                                  \\     if (__s == 1)                                  \\       __p = g_##func (__n);                        \\     else if (__builtin_constant_p (__n) &&         \\              (__s == 0 || __n <= G_MAXSIZE \/ __s)) \\       __p = g_##func (__n * __s);                  \\     else                                           \\       __p = g_##func##_n (__n, __s);               \\     __p;                                           \\   }))<\/code><\/pre>\n
  For each use of this macro, the analyzer issues the V773 warning (Visibility scope of the ‘__p’ pointer was exited without releasing the memory. A memory leak is possible). The g_new<\/i> macro is defined in the glib library, it uses the _g_new<\/i> macro, and this macro in turn uses another G_GNUC_EXTENSION<\/i> macro that tells the GCC compiler to skip warnings about non-standard code. It is this non-standard code that triggers the analyzer’s warning. Just look as the last but one line of code. In fact, the macro is valid. There were 848 warnings of this type, which means that almost half of the warnings occur in just one single place in the code.<\/p>\n
  All these unnecessary warnings can be easily removed<\/a> using the analyzer settings. However, this particular case, which occurred when writing the article, is the reason for our team to slightly refine the logic of the analyzer for such situations.<\/p>\n
  Thus, a large number of warnings doesn’t always indicate poor code quality. However, there are some really suspicious places. Well, let’s get down to reviewing the warnings.<\/p>\n
  Warning N1<\/b><\/p>\n
  V517<\/a> The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 2395, 2397. megasas.c 2395<\/p>\n
#define MEGASAS_MAX_SGE 128             \/* Firmware limit *\/ .... static void megasas_scsi_realize(PCIDevice *dev, Error **errp) {   ....   if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {     ....   } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {     ....   }   .... }<\/code><\/pre>\n
  Any use of «magic» numbers in the code is always suspicious. There are two conditions here, and at a first glance, they seem different, but if you look at the value of the MEGASAS_MAX_SGE<\/i> macro, it turns out that the conditions duplicate each other. Most likely, there is a typo and a different number should be written instead of 128. Sure, this is the problem with all «magic» numbers, one can easily mistype them. Using macros and constants will help a developer a lot in this case.<\/p>\n
  Warning N2<\/b><\/p>\n
  V523<\/a> The ‘then’ statement is equivalent to the ‘else’ statement. cp0_helper.c 383<\/p>\n
target_ulong helper_mftc0_cause(CPUMIPSState *env) {   ....   CPUMIPSState *other = mips_cpu_map_tc(env, &other_tc);    if (other_tc == other->current_tc) {     tccause = other->CP0_Cause;   } else {     tccause = other->CP0_Cause;   }   .... }<\/code><\/pre>\n
  In the code above, then<\/i> and else<\/i> bodies of the if<\/i> statement are identical. Most likely, it’s copy-paste. The author just copied the body of then<\/i> branch, and forgot to fix it. As far as I can see, env<\/i> should have been used instead of the other<\/i> object. Fixing this suspicious place can look like this: <\/p>\n
if (other_tc == other->current_tc) {   tccause = other->CP0_Cause; } else {   tccause = env->CP0_Cause; }<\/code><\/pre>\n
  Only the developers of this code can clearly say how it should actually be. Another similar fragment:<\/p>\n