Clever Geek Handbook

英特尔PMDK库集合的静态代码分析和不是错误的错误

PVS-Studio，PMDK


我们提供了使用PVS-Studio分析仪检查开放PMDK库的集合，这些库旨在用于开发和调试具有非易失性存储器支持的应用程序。实际上，为什么不呢？此外，这是一个用C和C ++编写的小项目，代码总大小约为170 KLOC，不包括注释。这意味着查看分析结果不会花费很多时间和精力。我们走吧。



要分析源代码，将使用PVS-Studio工具版本7.08。很自然，我们博客的读者很早就熟悉我们的工具，因此我将不再赘述。对于第一次来我们这里的人，建议参考文章“如何快速查看PVS-Studio分析仪针对C和C ++代码发出的有趣警告？ ”，然后尝试使用该分析仪的免费试用版。



这次，我将浏览PMDK项目内部，并向您介绍我注意到的错误和缺点。以我的内心感觉，它们并不多，这表明该项目代码的质量很好。有趣的是，我们发现了错误代码的几个片段，这些片段仍然可以正确运行:)。通过进一步的叙述，我的意思将变得更加清楚。



总而言之，PMDK是开放源代码库和工具的集合，旨在简化支持非易失性内存的应用程序的开发，调试和管理。此处有更多详细信息：PMDK简介。资料来源：pmdk。



让我们看看我可以在其中发现哪些错误和不足。我必须马上说，我在分析报告时并非总是那么专心，可能已经错过了很多。因此，我敦促项目作者在修复缺陷时不要仅仅受到本文的指导，而是要自己仔细检查代码。对于写一篇文章，我写出的内容，看看警告列表，对我来说就足够了:)。



无效的代码



内存分配大小



当程序表现出不正常的行为时，程序员通常会花时间调试代码。但是，有时在某些情况下程序可以正常运行，但是代码中包含错误。程序员只是幸运的，错误并没有显现出来。在PMDK项目中，我一次遇到了几种有趣的情况，因此决定将它们放到单独的章节中。 



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


PVS-Studio警告：V568奇怪的是'sizeof（）'运算符会评估指向类的指针的大小，而不是'pop'类对象的大小。util_ctl.c 717



一个经典的错字，导致分配了错误的内存量。sizeof运算符将不返回结构的大小，而是返回指向该结构的指针的大小。正确的选择是： 



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


要么 



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


但是，此错误编写的代码非常有用。关键是池结构仅包含一个指针： 



struct pool {
  struct ctl *ctl;
};


事实证明，该结构所占用的空间与指针完全一样。事情很好。



线长



让我们继续下一种情况，其中再次使用sizeof运算符产生了错误。 



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警告：V579 [CWE-687] memcpy_fn函数接收指针及其大小作为参数。这可能是一个错误。检查第三个论点。 pmem2_map_prot.c 513



指向特殊复制功能的指针用于复制字符串。请注意此函数的调用，或者要注意其第三个参数。



程序员假定sizeof运算符将计算字符串文字的大小。但是，实际上，指针大小是重新计算的。



幸运的是，该字符串由8个字符组成，并且在构建64位应用程序时其大小与指针相同。结果，字符串“ No code”的所有8个字符。将被成功复制。



实际上，情况更加复杂和有趣。该错误的解释取决于您是否要复制终端零。让我们考虑两种情况。



方案1.必须复制终端零。那我就错了，这不是一个没有表现出来的无害错误。不是复制9个字节，而是仅复制8个字节。没有终端零，因此无法预测后果。在这种情况下，可以通过如下更改常量字符串initial_state的定义来更正代码： 



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


现在sizeof（initial_state）的值为9。



方案2。根本不需要端子0。例如，下面您可以看到以下代码行： 



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


如您所见，strlen函数将返回值8，并且比较中不包含端子零。那真的很幸运，一切都很好。



位移



下一个示例与按位移位操作有关。 



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警告：V610 [CWE-758]未指定的行为。检查移位运算符“ >>”。左操作数“〜0”为负。clo.cpp 205



将负值向右移的结果取决于编译器的实现。因此，尽管这样的代码可以在所有当前现有的应用程序编译模式下正常工作并按预期工作，但是仍然很幸运。



作业优先



并考虑与操作优先级有关的最后一种情况。 



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


PVS-Studio警告：V634 [CWE-783]'*'操作的优先级高于'<<'操作的优先级。表达式中可能应使用括号。bttcreate.c 204



为了获得等于20 MB的常量，程序员决定执行以下操作：



  • 将1移位20位以获得1048576，即 1 MB。
  • 1 MB乘以20。


换句话说，程序员认为计算是这样进行的：（20 *（1UL << 20））



但实际上，乘法运算符的优先级高于移位运算符的优先级，并且表达式的计算方式如下：（（20 * 1UL）<< 20）。



同意，程序员几乎不希望按这样的顺序对表达式求值。将20乘以1是没有意义的。因此，当代码无法按程序员预期的方式工作时，就是这种情况。



但是此错误不会以任何方式显现出来。不管你怎么写：



  • （20 * 1UL << 20）
  • （20 *（1UL << 20））
  • （（20 * 1UL）<< 20）


结果总是一样的！所需的值始终是20971520，并且程序可以正常正常运行。



其他错误



括号放在错误的位置



#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警告：V593 [CWE-783]考虑查看“ A = B == C”类型的表达式。该表达式的计算公式如下：“ A =（B == C）”。ut.c 641



在这里仔细看： 



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


程序员希望将NtQuerySystemInformation函数返回的值存储在状态变量中，然后将其与常量进行比较。 程序员最有可能知道比较运算符（==）的优先级高于赋值运算符（=），因此应使用括号。但是他密封了自己，把他们放在错误的地方。结果，括号无济于事。正确的代码： 







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


由于此错误，UT_ASSERT宏将永远无法工作。实际上，状态变量始终包含比较结果，即false（0）或true（1）。因此，条件（[0..1]> = 0）始终为真。



潜在的内存泄漏



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警告：V773 [CWE-401]在不释放“输入”指针的情况下退出了该功能。可能发生内存泄漏。pmemobjcli.c 238



如果oidp变成空指针，则通过调用strdup函数创建的字符串的副本将丢失。最好在分配内存之前推迟检查： 



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


或者，您可以显式释放内存： 



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


潜在的溢出



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警告：V1028 [CWE-190]可能的溢出。考虑转换操作数，而不是结果。memcpy_common.c 62



明确将加法的结果强制转换为os_off_t类型是没有意义的。首先，它不能防止在添加两个int值时可能发生的潜在溢出。其次，无论如何，添加的结果都可以隐式扩展为os_off_t类型。显式类型转换只是多余的。



我认为这样写会更正确： 



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


在此，将size_t类型的无符号值转换为有符号值（这样，编译器就不会发出警告）。同时，添加过程中绝对不会出现溢出。



错误的溢出保护



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警告：V547 [CWE-570]表达式'rel_wait <0'始终为false。无符号类型的值永远不会小于0. os_thread_windows.c 359



我不太清楚该检查应针对的情况，但是仍然无法正常工作。rel_wait变量为无符号DWORD类型。这意味着比较rel_wait <0是没有意义的，因为结果始终为true。



缺少验证是否已成功分配内存的验证



使用assert宏检查是否已分配内存，如果编译了应用程序的Release版本，则不会执行任何操作。因此，可以说没有处理malloc函数返回NULL的情况。例： 



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警告：V575 [CWE-628]潜在的空指针已传递到“ memcpy”功能。检查第一个参数。检查行：340，338。rtree_map.c 340



在其他地方甚至没有断言： 



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警告：V522 [CWE-690]可能会取消引用潜在的空指针“工人”。检查行：126，124。pi.c 126



我至少计算了37个这样的代码片段。因此，我认为没有理由在文章中列出所有这些内容。



乍一看，缺乏检查可以被认为只是草率，并说这是一种带有气味的代码。我不同意这个立场。程序员低估了丢失此类支票的危险。尝试取消引用空指针时，它不一定会立即在程序崩溃时表现出来。结果可能会更加奇怪和危险，特别是在多线程程序中。为了更详细地了解问题所在以及为什么需要进行检查，我强烈建议大家阅读以下文章：检查malloc函数返回的内容为何很重要。



嗅到代码



两次通话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警告：V586 [CWE-675]两次调用“ CloseHandle”功能以释放同一资源。pmem2_map.c 76



查看此代码和PVS-Studio警告，很明显，没有什么是清楚的。在哪里可以再次调用CloseHandle？为了找到答案，让我们看一下UT_ASSERTne宏的实现。 



#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)


它并没有变得更加清晰。什么是UT_ASSERT_COMPILE_ERROR_ON？什么是UT_ASSERTne_rt？



我不会在每个文章的描述中弄乱文章，也不会折磨读者，迫使读者将一个宏插入其他人的脑海中。让我们立即查看从预处理文件中获取的打开代码的最终版本。 



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


让我们删除始终为假的条件（0 && 0），以及通常所有不相关的条件。原来： 



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


手柄已关闭。如果发生错误，则会生成调试消息，并再次获取错误代码，将为相同的无效句柄调用CloseHandle。



错误，种类，不是。由于该句柄无效，因此可以为它两次调用CloseHandle函数。但是，此代码是无味的。从思想上讲，只调用一次该函数并保存其返回的状态，然后在必要时在消息中显示其值会更正确。



实现接口不匹配（删除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);
  }
  ....
}


分析器显示以下消息：V530 [CWE-252]需要使用功能“ status_msg_info_and_question”的返回值。check_util.c 293



原因是从分析器的角度来看，status_msg_info_and_question函数不会更改其外部对象（包括传递的常量字符串）的状态。那些。该函数只是计算一些东西并返回结果。如果是这样，那么不使用此函数返回的结果很奇怪。而且，尽管这次分析仪是错误的，但它指向的是带有气味的代码。让我们看看被调用的status_msg_info_and_question函数如何工作。 



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


调用strchr函数时，将隐式删除常量。事实是，在C中这样声明： 



char * strchr ( const char *, int );


不是最好的解决方案。但是C语言才是真正的:)。



分析器感到困惑，并且不了解所传递的字符串实际上正在更改。如果是这样，则返回值不是最重要的事情，您不能使用它。



但是，尽管分析仪感到困惑，但它指向的是带有气味的代码。令人困惑的解析器还可能使维护代码的人困惑。最好通过删除const更诚实地声明该函数： 



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


这样一来，意图就更加清晰了，分析仪将保持沉默。



过于复杂的代码



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警告：V547 [CWE-571]表达式'blocks [i]'始终为true。heap.c 1054



如果block [i] == NULL，那么将触发继续语句，并且循环将开始下一次迭代。因此，重新检查元素块[i]没有意义，并且三元运算符是多余的。该代码可以简化： 



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


可疑使用空指针



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警告：V1004 [CWE-119]在针对nullptr进行验证之后，不安全地使用了'（char *）mt-> BaseAddress'指针。检查线：226，235 win_mmap.c 235



的MT-> BaseAddress指针可以为空，由单向证明： 



if (mt->BaseAddress != NULL)


但是，下面的该指针已用于算术运算中，无需检查。例如，在这里： 



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


将会收到一些大的整数值，它实际上是mt-> EndAddress指针的值。这可能不是一个错误，但所有看起来都非常可疑，在我看来，应该重新检查代码。气味在于这样一个事实，即代码难以理解，并且显然缺少解释性注释。



全局变量的简称



我相信，如果代码包含带有短名称的全局变量，则代码会闻起来。密封起来很容易，并且在某些函数中不小心使用了局部变量而不是全局变量。例： 



static struct critnib *c;


PVS-Studio针对此类变量的警告：



  • V707为全局变量使用短名称被认为是不好的做法。建议重命名“ ri”变量。第131章
  • V707为全局变量使用短名称被认为是不好的做法。建议重命名“ c”变量。obj_critnib_mt.c 56
  • V707为全局变量使用短名称被认为是不好的做法。建议重命名“ Id”变量。obj_list.h 68
  • V707为全局变量使用短名称被认为是不好的做法。建议重命名“ Id”变量。obj_list.c 34


奇怪



PVS-Studio：函数中的奇怪代码


我遇到的最奇怪的代码是在do_memmove函数中。分析仪给出了两个肯定的结论，这表明非常严重的错误，或者我根本不理解我的意思。由于代码很奇怪，因此我决定考虑在本文的单独部分中发布的警告。因此，此处发出第一个警告。 



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警告：V549 [CWE-688]“ memmove”功能的第一个参数等于第二个参数。memmove_common.c 71



请注意，函数的第一个和第二个参数相同。因此，该功能实际上不执行任何操作。我想到什么选择：



  • 我想“触摸”内存块。但这会在现实中发生吗？优化编译器是否会删除将内存块复制到自身的代码？
  • 这是对memmove函数的某种单元测试。
  • 该代码包含一个错字。


这是同一个功能中同样奇怪的代码段： 



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警告：V575 [CWE-628]“记忆”功能处理“ 0”元素。检查第三个论点。memmove_common.c 82



该函数移动0个字节。它是什么？单元测试？错别字？



对我来说，这段代码是难以理解和奇怪的。



为什么要使用代码分析器？



似乎因为发现了很少的错误，所以将分析器引入代码开发过程是不合理的。但是，使用静态分析工具的重点不是一次性检查，而是在编写代码的阶段定期检测错误。否则，将以更昂贵，更慢的方式（调试，测试，用户检查等）检测这些错误。我建议您熟悉这篇文章“由于未使用静态代码分析而无法找到错误”，对此文章进行了详细说明。然后到我们的网站下载并尝试使用PVS-Studio检查您的项目。



感谢您的关注！





如果您想与讲英语的读者分享本文，请使用翻译链接：Andrey Karpov。Intel对PMDK库集合的静态代码分析和不是实际错误的错误。


More articles:


All Articles