{"id":409185,"date":"2024-06-29T20:38:16","date_gmt":"2024-06-29T20:38:16","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=409185"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=409185","title":{"rendered":"Static code analysis of the PMDK library collection by Intel and errors that are not actual errors<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n
\"PVS-Studio,<\/div>\n

We were asked to check a collection of open source PMDK libraries for developing and debugging applications with NVRAM support by PVS-Studio. Well, why not? Moreover, this is a small project in C and C++ with a total code base size of about 170 KLOC without comments. Which means, the results review won’t take much energy and time. Let’s go.
<\/a>
The PVS-Studio 7.08 tool will be used to analyze the source code. Of course, readers of our blog have long been familiar with our tool, so I won’t focus on it. For those who have visited us for the first time, I suggest you refer to the article «How to quickly check out interesting warnings given by the PVS-Studio analyzer for C and C++ code<\/a>?» and try<\/a> the free trial version of the analyzer.<\/p>\n

This time I will take a look inside the PMDK project and tell you about the errors and shortcomings that I’ve noticed. My inner feeling was telling me there weren’t many of them, which indicates a high quality of the project code. As for some peculiar things, I found several fragments of incorrect code, which nevertheless was working correctly :). What I mean will become clearer from the rest of the story.<\/p>\n

So PMDK is a collection of open source libraries and tools designed to simplify the development, debugging, and management of applications that support NVRAM. Check out more details here: PMDK Introduction<\/a>. The source code is available here: pmdk<\/a>.<\/p>\n

Let’s see what errors and shortcomings I can find in it. I must say straight away that I wasn’t always attentive when analyzing the report and could have missed a lot. Therefore, I urge the authors of the project not to be guided by this article when correcting defects, but to double-check the code themselves. As for me, to write the article, it will be enough to cite what I noted while viewing the list of warnings :).<\/p>\n

Incorrect code that works <\/h2>\n

<\/p>\n

Size of allocated memory<\/h3>\n

Programmers often spend time debugging code when the program doesn’t behave as it should. However, sometimes there are cases when the program works correctly, but the code contains an error. The programmer just got lucky, and the error doesn’t reveal itself. In the PMDK project, I stumbled upon several such interesting cases, so I decided to gather them together in a separate section.<\/p>\n

int main(int argc, char *argv[]) {   ....   struct pool *pop = malloc(sizeof(pop));   .... }<\/code><\/pre>\n
  PVS-Studio warning: V568 It’s odd that ‘sizeof()’ operator evaluates the size of a pointer to a class, but not the size of the ‘pop’ class object. util_ctl.c 717<\/p>\n
  A classic typo due to which the wrong amount of memory is allocated. The sizeof<\/i> operator will return the size of the pointer to the structure instead of the size of this structure. The correct version is:<\/p>\n
struct pool *pop = malloc(sizeof(pool));<\/code><\/pre>\n
  or<\/p>\n
struct pool *pop = malloc(sizeof(*pop));<\/code><\/pre>\n
  However, this incorrectly written code works fine. The fact is that the pool<\/i> structure contains exactly one pointer:<\/p>\n
struct pool {   struct ctl *ctl; };<\/code><\/pre>\n
  It turns out that the structure takes exactly as much space as the pointer. So that’s all right.<\/p>\n
String length<\/h3>\n
  Let’s move on to the next case where an error was made again using the sizeof<\/i> operator.<\/p>\n
typedef void *(*pmem2_memcpy_fn)(void *pmemdest, const void *src, size_t len,     unsigned flags);  static const char *initial_state = \"No code.\";  static int test_rwx_prot_map_priv_do_execute(const struct test_case *tc,   int argc, char *argv[]) {   ....   char *addr_map = pmem2_map_get_address(map);   map->memcpy_fn(addr_map, initial_state, sizeof(initial_state), 0);   .... }<\/code><\/pre>\n
  PVS-Studio warning: V579 [CWE-687] The memcpy_fn function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. pmem2_map_prot.c 513<\/p>\n
  To copy a string, a pointer to a special copy function is used. Note the call to this function, or rather its third argument.<\/p>\n
  The programmer assumes that the sizeof<\/i> operator will calculate the size of the string literal. But, in fact, it is the size of the pointer that is calculated again.<\/p>\n
  The lucky thing is that the string consists of 8 characters, and its size matches the size of the pointer if the 64-bit application is being built. As a result, all 8 characters of the string «No code.» will be copied successfully.<\/p>\n
  In fact, the situation is even more complicated and intriguing. The interpretation of this error depends on whether the author wanted to copy the terminal null or not. Let’s consider two scenarios.<\/p>\n
  Scenario 1.<\/b> Terminal null had to be copied. This way, I’m wrong and this is not just a harmless bug that doesn’t manifest itself. Only 8 bytes were copied, not 9 bytes. There is no terminal null, and the consequences can’t be predicted. In this case, one can correct the code by changing the definition of the initial_state<\/i> constant string as follows:<\/p>\n
static const char initial_state [] = \"No code.\";<\/code><\/pre>\n
  Now the value of sizeof(initial_state)<\/i> is 9.<\/p>\n
  Scenario 2.<\/b> Terminal null is not required at all. For example, you can see this line of code below:<\/p>\n
UT_ASSERTeq(memcmp(addr_map, initial_state, strlen(initial_state)), 0);<\/code><\/pre>\n
  As you can see, the strlen<\/i> function returns 8 and terminal null is not involved in the comparison. Then it’s really good luck and all is well.<\/p>\n
Bitwise shift<\/h3>\n
  The following example is related to the bitwise shift operation.<\/p>\n
static int clo_parse_single_uint(struct benchmark_clo *clo, const char *arg, void *ptr) {   ....   uint64_t tmax = ~0 >> (64 - 8 * clo->type_uint.size);   .... }<\/code><\/pre>\n
  PVS-Studio warning: V610 [CWE-758] Unspecified behavior. Check the shift operator ‘>>’. The left operand ‘~0’ is negative. clo.cpp 205<\/p>\n
  The result of shifting the negative value to the right depends on the compiler implementation. Therefore, although this code may work correctly and expectedly under all currently existing application compilation modes, it is still a piece of luck.<\/p>\n
Operation precedence<\/h3>\n
  And let’s look at the last case related to the operation precedence.<\/p>\n
#define BTT_CREATE_DEF_SIZE  (20 * 1UL << 20) \/* 20 MB *\/<\/code><\/pre>\n
  PVS-Studio warning: V634 [CWE-783] The priority of the ‘*’ operation is higher than that of the ‘<<‘ operation. It’s possible that parentheses should be used in the expression. bttcreate.c 204<\/p>\n
  To get a constant equal to 20 MB, the programmer decided to follow these steps:<\/p>\n