# Checking the code of Valgrind dynamic analyzer by a static analyzer I should say right away, that the article was not written to show that static analysis works better than dynamic\. This statement would be incorrect, as well as the reverse idea\. The tools of static and dynamic analysis complement each other, they do not compete with each other\. Both of these methods have strengths and weaknesses\. Some errors cannot be detected by dynamic analyzers, some \- by static ones\. That's why I suggest treating this post as another demonstration of the abilities of PVS\-Studio, not the comparison of two methodologies\. ![0504_valgrind_check/image1.png](https://import.viva64.com/docx/blog/0504_valgrind_check/image1.png) ## The methodologies of dynamic and static analysis The source code contains some hints that can help you detect errors\. Let's look at a simple example: ```cpp char *str = foo(); if (str == '\0') ``` It is strange to compare the pointer not with _nullptr, NULL _or at least with _0_, but with a character literal _'\\0'_\. On the basis of this strange thing, a static analyzer can assume that they wanted to check not the fact that the pointer is 0, but that the string is empty, i\.e\. there was an intention to check if there is a [terminal null](https://pvs-studio.com/en/blog/terms/0088/) in the beginning of the string, but a programmer accidentally forgot to dereference the pointer\. Most likely, this will really be an error and the correct code will be as follows: ```cpp char *str = foo(); if (*str == '\0') ``` This information gets lost during the compilation and the dynamic analyzer is not able to detect this bug\. From the point of view of the dynamic analyzer, the pointer is verified against _NULL_, so there is nothing to worry about\. Another weakness of a dynamic analyzer is in the necessity to execute the code containing an error\. Sometimes it is very hard to do for a big number of code fragments\. I'll explain this using a code fragment taken from a real application: ```cpp ADOConnection* piTmpConnection = NULL; hr = CoCreateInstance( CLSID_DataLinks, NULL, CLSCTX_INPROC_SERVER, IID_IDataSourceLocator, (void**)&dlPrompt ); if( FAILED( hr ) ) { piTmpConnection->Release(); dlPrompt->Release( ); return connstr; } ``` If the function _CoCreateInstance _was executed with an error, then we'll get the dereference of the _piTmpConnection _null pointer\. In fact, the string _piTmpConnection\-\>Release\(\);_ is just redundant here, because no connection was created here\. It could be rather troublesome to detect such a situation with the help of a dynamic analyzer, as we'll have to simulate a situation when the function _CoCreateInstance_ returns the error status\. It's not easy to do this\. Theoretically, the static analyzer has information about the code, and therefore is capable of finding more bugs than a dynamic analyzer\. In practice, the possibility of static analyzers are limited by the available memory and the acceptable time of work\. In other words, a static analyzer can consider how code will work under all possible variants of input data\. But it will take a little less than 150 years on the cluster, where it will need an incredible amount of installed memory\. As a result, in practice, static analyzers aren't able to detect a lot of error types\. For example, they do not notice leaks, if the pointer is passed between a lot of functions\. In turn, the dynamic analyzers greatly cope with such tasks, regardless of the code complexity\. ## The analysis results We [regularly check](https://pvs-studio.com/en/blog/inspections/) various projects to spread the word about static analysis methodology in general, and about our tool PVS\-Studio in particular, so I couldn't miss a chance to check the Valgrind project\. It is a kind of a challenge to us to find errors in it\. This is a high\-quality, well\-tested project that is already being checked by Coverity\. In general, I am sure that this code was checked by enthusiasts and various tools\. Even several errors found would be a great success\. Let's see if there was anything interesting that PVS\-Studio managed to find in the code of Valgrind\. ```cpp static void lk_fini(Int exitcode) { .... VG_(umsg)(" taken: %'llu (%.0f%%)\n", taken_Jccs, taken_Jccs * 100.0 / total_Jccs ?: 1); .... } ``` PVS\-Studio warning: V502 Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '/' operator\. lk\_main\.c 1014 The operator ?: is very treacherous and it should be used very carefully\. I have discussed this topic in the fourth chapter of my [small e\-book](https://pvs-studio.com/en/blog/posts/cpp/0391/), which I recommend to have a look at\. Let's see why this code is suspicious\. I think the programmer wanted to protect himself from dividing by zero\. That's why, if the variable _total\_Jccs is 0, _then the division should be by _1_\. The code was intended to work like this: ```cpp taken_Jccs * 100.0 / (total_Jccs ?: 1) ``` However, the precedence of the ?: operator is lower than of the division and multiplication operators\. Therefore, the expression is evaluated as follows: ```cpp (taken_Jccs * 100.0 / total_Jccs) ?: 1 ``` However, perhaps the code works exactly as intended\. Even if it so, it's better to add brackets, so that other programmers don't get confused in the future, if there is an error here or not\. ```cpp static Bool doHelperCall (....) { .... UInt nVECRETs = 0; .... vassert(nVECRETs == (retTy == Ity_V128 || retTy == Ity_V256) ? 1 : 0); .... } ``` PVS\-Studio warning: V502 Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '\=\=' operator\. host\_arm\_isel\.c 795 In fact, this is an interesting case\. The ?: operator is used incorrectly, but still, the code is correct\. The check was meant to work like this: ```cpp nVECRETs == ((retTy == Ity_V128 || retTy == Ity_V256) ? 1 : 0) ``` But it works like this: ```cpp (nVECRETs == (retTy == Ity_V128 || retTy == Ity_V256)) ? 1 : 0 ``` The funny thing is that if you look closely, you can see that these checks are equivalent\. The result will be the same\. Similar issues can be found here: * V502 Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '\=\=' operator\. host\_arm64\_isel\.c 737 * V502 Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '\=\=' operator\. host\_mips\_isel\.c 611 ```cpp typedef ULong DiOffT; typedef struct { Bool fromC; DiOffT off; SizeT size; SizeT used; UChar data[]; } CEnt; static Bool is_sane_CEnt (....) { .... CEnt* ce = img->ces[i]; .... if (!(ce->size == CACHE_ENTRY_SIZE)) goto fail; if (!(ce->off >= 0)) goto fail; // <= if (!(ce->off + ce->used <= img->real_size)) goto fail; .... } ``` PVS\-Studio warning: V547 Expression 'ce\-\>off \>\= 0' is always true\. Unsigned type value is always \>\= 0\. image\.c 147 The _off_ member is a variable of an unsigned type, which means it is always greater than or equal to zero\. Thus, the condition _\(\!\(ce\-\>off_ _\>\=_ _0\)\)_ is always false\. ```cpp static void sdel_Counts ( Counts* cts ) { memset(cts, 0, sizeof(Counts)); free(cts); } ``` PVS\-Studio warning: V597 The compiler could delete the 'memset' function call, which is used to flush 'cts' object\. The memset\_s\(\) function should be used to erase the private data\. cg\_merge\.c 324 Perhaps, to simplify the error search in Valgrind, the memory is filled with zeros before freeing\. However, in the release\-version the compiler will likely to remove the call of the _memset_ function, as the buffer is not used anymore before the call of the _free_ function\. Similar fragments where the memory may not be zeroed: * V597 The compiler could delete the 'memset' function call, which is used to flush 'ffn' object\. The memset\_s\(\) function should be used to erase the private data\. cg\_merge\.c 263 * V597 The compiler could delete the 'memset' function call, which is used to flush 'cts' object\. The memset\_s\(\) function should be used to erase the private data\. cg\_merge\.c 332 * V597 The compiler could delete the 'memset' function call, which is used to flush 'cpf' object\. The memset\_s\(\) function should be used to erase the private data\. cg\_merge\.c 394 ```cpp static Bool dis_AdvSIMD_scalar_shift_by_imm(DisResult* dres, UInt insn) { .... ULong nmask = (ULong)(((Long)0x8000000000000000ULL) >> (sh-1)); .... } ``` PVS\-Studio warning: V610 Unspecified behavior\. Check the shift operator '\>\>'\. The left operand '\(\(Long\) 0x8000000000000000ULL\)' is negative\. guest\_arm64\_toIR\.c 9428 If the shifted operand has a negative value, the resulting is implementation\-defined\. Thus, we are dealing with dangerous code\. Now let's consider a situation when the dereference of the pointer is written before its check against _NULL_: ```cpp PRE(xsm_op) { struct vki_xen_flask_op *op = (struct vki_xen_flask_op *)ARG1; PRINT("__HYPERVISOR_xsm_op ( %u )", op->cmd); // <= PRE_MEM_READ("__HYPERVISOR_xsm_op", ARG1, sizeof(vki_uint32_t) + sizeof(vki_uint32_t)); if (!op) // <= return; .... } ``` PVS\-Studio warning: V595 The 'op' pointer was utilized before it was verified against nullptr\. Check lines: 350, 360\. syswrap\-xen\.c 350 Similar cases: * V595 The 'sysctl' pointer was utilized before it was verified against nullptr\. Check lines: 568, 578\. syswrap\-xen\.c 568 * V595 The 'domctl' pointer was utilized before it was verified against nullptr\. Check lines: 710, 722\. syswrap\-xen\.c 710 ```cpp Bool ML_(read_elf_debug_info) ( struct _DebugInfo* di ) { .... if (inrw && sdynbss_present) { vg_assert(di->sbss_present); sdynbss_present = False; vg_assert(di->sbss_svma + di->sbss_size == svma); di->sbss_size += size; .... } else // <= if (inrw && !di->sbss_present) { di->sbss_present = True; di->sbss_svma = svma; di->sbss_avma = svma + inrw->bias; .... } ``` PVS\-Studio warning: V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics\. readelf\.c 2231 The _else_ keyword in the code looks very suspicious\. The code is not aligned in accordance with the logic of his work\. In addition, there is an empty line after _else_\. This suggests that we see the consequences of sloppy refactoring and _else_ is not needed here\. ```cpp static Bool doHelperCallWithArgsOnStack (....) { .... if (guard) { if (guard->tag == Iex_Const && guard->Iex.Const.con->tag == Ico_U1 && guard->Iex.Const.con->Ico.U1 == True) { /* unconditional -- do nothing */ } else { goto no_match; //ATC cc = iselCondCode( env, guard ); } } .... } ``` PVS\-Studio warning: V779 Unreachable code detected\. It is possible that an error is present\. host\_arm\_isel\.c 461 The code line ```cpp cc = iselCondCode( env, guard ); ``` will never get executed\. ```cpp void reset_valgrind_sink(const char *info) { if (VG_(log_output_sink).fd != initial_valgrind_sink.fd && initial_valgrind_sink_saved) { VG_(log_output_sink).fd = initial_valgrind_sink.fd; VG_(umsg) ("Reset valgrind output to log (%s)\n", (info = NULL ? "" : info)); } } ``` PVS\-Studio warning: V547 Expression '\(\(void \*\) 0\)' is always false\. server\.c 110 The analyzer warning may look strange and needs clarification\. We are interested in the following statement: ```cpp (info = NULL ? "" : info)) ``` The macro _NULL_ expands to _\(\(void \*\) 0\)_ and we get: ```cpp (info = ((void *) 0) ? "" : info)) ``` The precedence of the ?: operator is higher than of the \= operator, that's why the evaluations are done in the following way: ```cpp (info = (((void *) 0) ? "" : info))) ``` Perhaps, you would agree that the condition _\(\(void \*\) 0\) _for the operator _?:_ looks strange; PVS\-Studio also warns us about it\. Apparently, we are dealing with a typo and the code should be as follows: ```cpp (info == NULL ? "" : info)) ``` And the last code fragment for today: ```cpp void genReload_TILEGX ( /*OUT*/ HInstr ** i1, /*OUT*/ HInstr ** i2, HReg rreg, Int offsetB ) { TILEGXAMode *am; vassert(!hregIsVirtual(rreg)); am = TILEGXAMode_IR(offsetB, TILEGXGuestStatePointer()); switch (hregClass(rreg)) { case HRcInt64: *i1 = TILEGXInstr_Load(8, rreg, am); break; case HRcInt32: *i1 = TILEGXInstr_Load(4, rreg, am); break; default: ppHRegClass(hregClass(rreg)); vpanic("genReload_TILEGX: unimplemented regclass"); break; } } ``` PVS\-Studio warning: V751 Parameter 'i2' is not used inside function body\. host\_tilegx\_defs\.c 1223 I think the programmer forgot to write _NULL_ by the address _i2_, as it was done in other similar functions: ```cpp *i1 = *i2 = NULL; ``` There is a similar bug here: V751 Parameter 'i2' is not used inside function body\. host\_mips\_defs\.c 2000 ## Conclusion Thank you for attention\. Try PVS\-Studio static code analyzer for Linux\. * Download: [PVS\-Studio for Linux](https://pvs-studio.com/en/pvs-studio/download/) * [Contact us](https://pvs-studio.com/en/about-feedback/) to get a temporary license key\. * Manual: [How to run PVS\-Studio on Linux](https://pvs-studio.com/en/docs/manual/0036/) Here is information for Windows developers: [PVS\-Studio for Windows](https://pvs-studio.com/en/pvs-studio/download/)\. For them everything is a little easier\. They can simply install the plugin for Visual Studio and check their C, C\+\+ and C\# projects using a demo\-version\.