Clever Geek Handbook

Static Code Analysis of Intel's PMDK Library Collection and Errors That Are Not Errors

PVS-Studio, PMDK


We were offered to use the PVS-Studio analyzer to check the collection of open PMDK libraries intended for developing and debugging applications with non-volatile memory support. Actually, why not. Moreover, this is a small project in C and C ++ with a total code base size of about 170 KLOC, excluding comments. This means that reviewing the analysis results will not take much time and effort. Let's go.



To analyze the source code, the PVS-Studio tool version 7.08 will be used. Readers of our blog, naturally, have long been familiar with our tool, and I will not dwell on it. For those who came to us for the first time, I suggest referring to the article " How to quickly view interesting warnings that are issued by the PVS-Studio analyzer for C and C ++ code? " And try the free trial version of the analyzer.



This time I will look inside the PMDK project and tell you about the errors and shortcomings I noticed. In my inner feeling, there were few of them, which speaks of the good quality of the code of this project. Of the interesting things, we can note that several fragments of incorrect code were found, which nevertheless works correctly :). What I mean will become clearer from further narration.



In summary, the PMDK is a collection of open source libraries and tools designed to simplify the development, debugging, and management of nonvolatile memory-enabled applications. More details here: PMDK Introduction . Sources here: pmdk .



Let's see what mistakes and shortcomings I can find in it. I must say right away that I was far from always attentive when analyzing the report and could have missed a lot. Therefore, I urge the authors of the project not to be guided exclusively by this article when fixing defects, but to double-check the code yourself. And for writing an article, what I wrote out, looking at the list of warnings, will be enough for me :).



Wrong code that works



Memory allocation size



It is not uncommon for programmers to spend time debugging code when the program does not behave the way it should. However, sometimes there are situations when the program works correctly, but the code contains an error. The programmer is just lucky, and the error does not manifest itself. In the PMDK project, I encountered several such interesting situations at once and therefore decided to put them together in a separate chapter. 



int main(int argc, char *argv[])
{
  ....
  struct pool *pop = malloc(sizeof(pop));
  ....
}


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



A classic typo that causes the wrong amount of memory to be allocated. The sizeof operator will return not the size of the structure, but the size of the pointer to this structure. The correct option would be: 



struct pool *pop = malloc(sizeof(pool));


or 



struct pool *pop = malloc(sizeof(*pop));


However, this incorrectly written code works great. The point is that the pool structure contains exactly one pointer: 



struct pool {
  struct ctl *ctl;
};


It turns out that the structure takes up exactly as much as the pointer. Things are good.



Line length



Let's move on to the next case, where the error was again made using the sizeof operator . 



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);
  ....
}


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



A pointer to a special copy function is used to copy a string. Pay attention to the call of this function, or rather to its third argument.



The programmer assumes that the sizeof operator will calculate the size of the string literal. But, in fact, the size of the pointer is calculated again.



Luckily, the string consists of 8 characters, and its size is the same as the pointer if building a 64-bit application. As a result, all 8 characters of the string "No code." will be copied successfully.



In fact, the situation is even more complicated and interesting. The interpretation of this error depends on whether you wanted to copy terminal zero or not. Let's consider two scenarios.



Scenario 1. It was necessary to copy terminal zero. Then I am wrong, and this is not at all a harmless mistake that does not manifest itself. Not 9 bytes were copied, but only 8. There is no terminal zero, and the consequences cannot be predicted. In this case, the code can be corrected by changing the definition of the constant string initial_state as follows: 



static const char initial_state [] = "No code.";


Now the sizeof (initial_state) value is 9.



Scenario 2. Terminal zero is not required at all. For example, below you can see the following line of code: 



UT_ASSERTeq(memcmp(addr_map, initial_state, strlen(initial_state)), 0);


As you can see, the strlen function will return the value 8 and terminal zero is not involved in the comparison. Then it's really luck and all is well.



Bit shift



The next example is related to a bitwise shift operation. 



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);
  ....
}


PVS-Studio warning: V610 [CWE-758] Unspecified behavior. Check the shift operator '>>'. The left operand '~ 0' is negative. clo.cpp 205



The result of shifting a negative value to the right depends on the compiler implementation. Therefore, although such code may work correctly and as expected under all currently existing application compilation modes, it is still luck.



Priority of operations



And consider the last case related to the priority of operations. 



#define BTT_CREATE_DEF_SIZE  (20 * 1UL << 20) /* 20 MB */


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



To get a constant equal to 20 MB, the programmer decided to do the following:



  • Shifted 1 by 20 bits to get the value 1048576, i.e. 1 MB.
  • Multiplied 1 MB by 20.


In other words, the programmer thinks that the calculations are performed like this: (20 * (1UL << 20))



But in fact, the priority of the multiplication operator is higher than that of the shift operator, and the expression is evaluated like this: ((20 * 1UL) << 20).



Agree, the programmer hardly wanted the expression to be evaluated in such a sequence. There is no point in multiplying 20 by 1. So this is the case when the code does not work the way the programmer intended.



But this error will not manifest itself in any way. It doesn't matter how you write:



  • (20 * 1UL << 20)
  • (20 * (1UL << 20))
  • ((20 * 1UL) << 20)


The result is always the same ! The desired value is always 20971520 and the program works perfectly correctly.



Other errors



Parenthesis in the wrong place



#define STATUS_INFO_LENGTH_MISMATCH 0xc0000004

static void
enum_handles(int op)
{
  ....
  NTSTATUS status;
  while ((status = NtQuerySystemInformation(
      SystemExtendedHandleInformation,
      hndl_info, hi_size, &req_size)
        == STATUS_INFO_LENGTH_MISMATCH)) {
    hi_size = req_size + 4096;
    hndl_info = (PSYSTEM_HANDLE_INFORMATION_EX)REALLOC(hndl_info,
        hi_size);
  }
  UT_ASSERT(status >= 0);
  ....
}


PVS-Studio warning: V593 [CWE-783] Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. ut.c 641



Take a close look here: 



while ((status = NtQuerySystemInformation(....) == STATUS_INFO_LENGTH_MISMATCH))


The programmer wanted to store in the status variable the value that the NtQuerySystemInformation function returns , and then compare it with a constant.



The programmer most likely knew that the comparison operator (==) has a higher priority than the assignment operator (=), and therefore parentheses should be used. But he sealed himself and put them in the wrong place. As a result, parentheses don't help in any way. Correct code: 



while ((status = NtQuerySystemInformation(....)) == STATUS_INFO_LENGTH_MISMATCH)


Because of this error, the UT_ASSERT macro will never work. Indeed, the status variable always contains the result of comparison, that is, false (0) or true (1). Hence the condition ([0..1]> = 0) is always true.



Potential memory leak



static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
  char *input = strdup(in);
  if (!input)
    return POCLI_ERR_MALLOC;

  if (!oidp)
    return POCLI_ERR_PARS;
  ....
}


PVS-Studio warning: V773 [CWE-401] The function was exited without releasing the 'input' pointer. A memory leak is possible. pmemobjcli.c 238



If oidp turns out to be a null pointer, the copy of the string created by calling the strdup function will be lost . It would be best to postpone the check before allocating memory: 



static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
  if (!oidp)
    return POCLI_ERR_PARS;

  char *input = strdup(in);
  if (!input)
    return POCLI_ERR_MALLOC;
  ....
}


Or, you can explicitly free the memory: 



static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
  char *input = strdup(in);
  if (!input)
    return POCLI_ERR_MALLOC;

  if (!oidp)
  {
    free(input);
    return POCLI_ERR_PARS;
  }
  ....
}


Potential overflow



typedef long long os_off_t;

void
do_memcpy(...., int dest_off, ....., size_t mapped_len, .....)
{
  ....
  LSEEK(fd, (os_off_t)(dest_off + (int)(mapped_len / 2)), SEEK_SET);
  ....
}


PVS-Studio warning: V1028 [CWE-190] Possible overflow. Consider casting operands, not the result. memcpy_common.c 62 It makes no sense to



explicitly cast the result of addition to the os_off_t type . First, it does not protect against potential overflow that can occur when adding two int values . Secondly, the result of the addition would have been perfectly implicitly expanded to the os_off_t type . Explicit type casting is simply redundant.



I think it would be more correct to write like this: 



LSEEK(fd, dest_off + (os_off_t)(mapped_len) / 2, SEEK_SET);


Here an unsigned value of the size_t type is converted into a signed value (so that there is no warning from the compiler). And at the same time, there will definitely not be an overflow during addition.



Incorrect overflow protection



static DWORD
get_rel_wait(const struct timespec *abstime)
{
  struct __timeb64 t;
  _ftime64_s(&t);
  time_t now_ms = t.time * 1000 + t.millitm;
  time_t ms = (time_t)(abstime->tv_sec * 1000 +
    abstime->tv_nsec / 1000000);

  DWORD rel_wait = (DWORD)(ms - now_ms);

  return rel_wait < 0 ? 0 : rel_wait;
}


PVS-Studio warning: V547 [CWE-570] Expression 'rel_wait <0' is always false. Unsigned type value is never <0. os_thread_windows.c 359



I'm not very clear about which situation the check should protect against, but it doesn't work anyway. The rel_wait variable is of unsigned DWORD type . This means that the comparison rel_wait <0 is meaningless, since the result is always true.



Lack of verification that memory has been successfully allocated



Checking that memory is allocated is done using assert macros , which do nothing if the Release version of the application is compiled. So we can say that there is no handling of the situation where malloc functions return NULL . Example: 



static void
remove_extra_node(TOID(struct tree_map_node) *node)
{
  ....
  unsigned char *new_key = (unsigned char *)malloc(new_key_size);
  assert(new_key != NULL);
  memcpy(new_key, D_RO(tmp)->key, D_RO(tmp)->key_size);
  ....
}


PVS-Studio warning: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 340, 338. rtree_map.c 340



Elsewhere there is not even an assert : 



static void
calc_pi_mt(void)
{
  ....
  HANDLE *workers = (HANDLE *) malloc(sizeof(HANDLE) * pending);
  for (i = 0; i < pending; ++i) {
    workers[i] = CreateThread(NULL, 0, calc_pi,
      &tasks[i], 0, NULL);
    if (workers[i] == NULL)
      break;
  }
  ....
}


PVS-Studio warning: V522 [CWE-690] There might be dereferencing of a potential null pointer 'workers'. Check lines: 126, 124. pi.c 126



I counted at least 37 such code fragments. So I see no reason to list all of them in the article.



At first glance, the lack of checks can be considered just sloppiness and say that this is a code with a smell. I do not agree with this position. Programmers underestimate the danger of not having such checks. A null pointer does not necessarily immediately manifest itself as a program crash when trying to dereference it. The consequences can be more bizarre and dangerous, especially in multi-threaded programs. To understand in more detail what is the matter and why checks are needed, I strongly recommend everyone to read the article "Why is it important to check what the malloc function returned ".



Smelling code



Double Call CloseHandle



static void
prepare_map(struct pmem2_map **map_ptr,
  struct pmem2_config *cfg, struct pmem2_source *src)
{
  ....
  HANDLE mh = CreateFileMapping(....);
  ....
  UT_ASSERTne(CloseHandle(mh), 0);
  ....
}


PVS-Studio warning: V586 [CWE-675] The 'CloseHandle' function is called twice for deallocation of the same resource. pmem2_map.c 76



Looking at this code and the PVS-Studio warning, it is clear that nothing is clear. Where can I call CloseHandle again ? To find the answer, let's look at the implementation of the UT_ASSERTne macro . 



#define UT_ASSERTne(lhs, rhs)\
  do {\
    /* See comment in UT_ASSERT. */\
    if (__builtin_constant_p(lhs) && __builtin_constant_p(rhs))\
      UT_ASSERT_COMPILE_ERROR_ON((lhs) != (rhs));\
    UT_ASSERTne_rt(lhs, rhs);\
  } while (0)


It didn't become much clearer. What is UT_ASSERT_COMPILE_ERROR_ON ? What is UT_ASSERTne_rt ?



I will not clutter up the article with a description of each macro and torment the reader, forcing him to insert one macros into others in his head. Let's see at once the final version of the opened code taken from the preprocessed file. 



do {
  if (0 && 0) (void)((CloseHandle(mh)) != (0));
  ((void)(((CloseHandle(mh)) != (0)) ||
    (ut_fatal(".....", 76, __FUNCTION__, "......: %s (0x%llx) != %s (0x%llx)",
              "CloseHandle(mh)", (unsigned long long)(CloseHandle(mh)), "0",
              (unsigned long long)(0)), 0))); } while (0);


Let's remove the always false condition (0 && 0) and, in general, everything irrelevant. It turns out: 



((void)(((CloseHandle(mh)) != (0)) ||
  (ut_fatal(...., "assertion failure: %s (0x%llx) != %s (0x%llx)",
            ....., (unsigned long long)(CloseHandle(mh)), .... ), 0)));


The handle is closed. If an error occurs, a debug message is generated and, to get the error code again, CloseHandle is called for the same invalid handle.



Errors, sort of, and no. Since the handle is invalid, then it's okay that the CloseHandle function is called twice for it . However, this code is odorless. It would be more ideologically correct to call the function only once and save the status that it returned, so that then, if necessary, display its value in the message.



Implementation interface inconsistency (removal of const)



static int
status_push(PMEMpoolcheck *ppc, struct check_status *st, uint32_t question)
{
  ....
  } else {
    status_msg_info_and_question(st->msg);            // <=
    st->question = question;
    ppc->result = CHECK_RESULT_ASK_QUESTIONS;
    st->answer = PMEMPOOL_CHECK_ANSWER_EMPTY;
    PMDK_TAILQ_INSERT_TAIL(&ppc->data->questions, st, next);
  }
  ....
}


The analyzer displays the message: V530 [CWE-252] The return value of function 'status_msg_info_and_question' is required to be utilized. check_util.c 293



The reason is that the status_msg_info_and_question function , from the analyzer's point of view, does not change the state of objects external to it, including the passed constant string. Those. the function just calculates something and returns the result. And if so, then it is strange not to use the result that this function returns. And, although the analyzer is wrong this time, it points to a code with a smell. Let's see how the called status_msg_info_and_question function works . 



static inline int
status_msg_info_and_question(const char *msg)
{
  char *sep = strchr(msg, MSG_SEPARATOR);
  if (sep) {
    *sep = ' ';
    return 0;
  }
  return -1;
}


When calling the strchr function , an implicit removal of the constant occurs. The fact is that in C it is declared like this: 



char * strchr ( const char *, int );


Not the best solution. But the C language is what it is :).



The analyzer got confused and did not understand that the passed string was actually changing. If so, then the return value is not the most important thing and you can not use it.



However, although the analyzer is confused, it points to a code with a smell. What is confusing the parser can also confuse the person who maintains the code. It would be better to declare the function more honestly by removing the const : 



static inline int
status_msg_info_and_question(char *msg)
{
  char *sep = strchr(msg, MSG_SEPARATOR);
  if (sep) {
    *sep = ' ';
    return 0;
  }
  return -1;
}


So the intentions are immediately clearer, and the analyzer will be silent.



Overcomplicated code



static struct memory_block
heap_coalesce(struct palloc_heap *heap,
  const struct memory_block *blocks[], int n)
{
  struct memory_block ret = MEMORY_BLOCK_NONE;

  const struct memory_block *b = NULL;
  ret.size_idx = 0;
  for (int i = 0; i < n; ++i) {
    if (blocks[i] == NULL)
      continue;
    b = b ? b : blocks[i];
    ret.size_idx += blocks[i] ? blocks[i]->size_idx : 0;
  }
  ....
}


PVS-Studio warning: V547 [CWE-571] Expression 'blocks [i]' is always true. heap.c 1054



If blocks [i] == NULL , then the continue statement will be triggered and the loop will start the next iteration. Therefore, re-checking the element blocks [i] does not make sense and the ternary operator is redundant. The code can be simplified: 



....
for (int i = 0; i < n; ++i) {
  if (blocks[i] == NULL)
    continue;
  b = b ? b : blocks[i];
  ret.size_idx += blocks[i]->size_idx;
}
....


Suspicious use of a null pointer



void win_mmap_fini(void)
{
  ....
  if (mt->BaseAddress != NULL)
    UnmapViewOfFile(mt->BaseAddress);
  size_t release_size =
    (char *)mt->EndAddress - (char *)mt->BaseAddress;
  void *release_addr = (char *)mt->BaseAddress + mt->FileLen;
  mmap_unreserve(release_addr, release_size - mt->FileLen);
  ....
}


PVS-Studio warning: V1004 [CWE-119] The '(char *) mt-> BaseAddress' pointer was used unsafely after it was verified against nullptr. Check lines: 226, 235. win_mmap.c 235



The mt-> BaseAddress pointer can be null, as evidenced by the check: 



if (mt->BaseAddress != NULL)


However, below this pointer is already used in arithmetic operations without checking. For example, here: 



size_t release_size =
  (char *)mt->EndAddress - (char *)mt->BaseAddress;


Some large integer value will be received, which is actually the value of the mt-> EndAddress pointer . This may not be a bug, but it all looks very suspicious, and it seems to me that the code should be rechecked. The smell lies in the fact that the code is incomprehensible and clearly lacks explanatory comments.



Short names of global variables



I believe that the code smells if it contains global variables with short names. It is easy to seal up and accidentally use not a local, but a global variable in some function. Example: 



static struct critnib *c;


PVS-Studio warnings for such variables:



  • V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'ri' variable. map.c 131
  • V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'c' variable. obj_critnib_mt.c 56
  • V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.h 68
  • V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.c 34


Strange



PVS-Studio: Strange code in a function


The strangest code I came across is in the do_memmove function . The analyzer gave two positives, which indicate either very serious errors, or that I simply do not understand what I mean. Since the code is very strange, I decided to consider the warnings issued in a separate section of the article. So the first warning is issued here. 



void
do_memmove(char *dst, char *src, const char *file_name,
    size_t dest_off, size_t src_off, size_t bytes,
    memmove_fn fn, unsigned flags, persist_fn persist)
{
  ....
  /* do the same using regular memmove and verify that buffers match */
  memmove(dstshadow + dest_off, dstshadow + dest_off, bytes / 2);
  verify_contents(file_name, 0, dstshadow, dst, bytes);
  verify_contents(file_name, 1, srcshadow, src, bytes);
  ....
}


PVS-Studio warning: V549 [CWE-688] The first argument of 'memmove' function is equal to the second argument. memmove_common.c 71



Note that the first and second arguments of the function are the same. Thus, the function does nothing in fact. What options come to my mind:



  • I wanted to "touch" the memory block. But will this happen in reality? Will the optimizing compiler remove the code that copies the memory block into itself?
  • This is some kind of unit test for the memmove function .
  • The code contains a typo.


And here is an equally strange snippet in the same function: 



void
do_memmove(char *dst, char *src, const char *file_name,
    size_t dest_off, size_t src_off, size_t bytes,
    memmove_fn fn, unsigned flags, persist_fn persist)
{
  ....
  /* do the same using regular memmove and verify that buffers match */
  memmove(dstshadow + dest_off, srcshadow + src_off, 0);
  verify_contents(file_name, 2, dstshadow, dst, bytes);
  verify_contents(file_name, 3, srcshadow, src, bytes);
  ....
}


PVS-Studio warning: V575 [CWE-628] The 'memmove' function processes '0' elements. Inspect the third argument. memmove_common.c 82



The function moves 0 bytes. What is it? Unit test? Typo?



For me, this code is incomprehensible and strange.



Why use code analyzers?



It may seem that since few errors were found, then the introduction of the analyzer into the code development process is unreasonable. But the point of using static analysis tools is not in one-time checks, but in the regular detection of errors at the stage of writing the code. Otherwise, these errors are detected in more expensive and slower ways (debugging, testing, user reviews, and so on). This idea is described in more detail in the article " Errors that static code analysis cannot find, because it is not used ", which I recommend to get acquainted with. And then come to our website to download and try PVS-Studio to check your projects.



Thank you for attention!





If you want to share this article with an English-speaking audience, please use the translation link: Andrey Karpov. Static code analysis of the PMDK library collection by Intel and errors that are not actual errors .


More articles:


All Articles