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

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.

我们被要求检查一组开源PMDK库，以开发和调试PVS-Studio支持NVRAM的应用程序。 好吧，为什么不呢？ 此外，这是一个用C和C ++编写的小项目，代码总大小约为170 KLOC，无注释。 这意味着结果审核不会花费很多精力和时间。 我们走吧。

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?" and try the free trial version of the analyzer.

PVS-Studio 7.08工具将用于分析源代码。 当然，我们博客的读者很早就熟悉我们的工具，因此我不会重点介绍它。 对于那些初次访问我们的人，建议您参考文章“ 如何快速检查PVS-Studio分析器针对C和C ++代码给出的有趣警告 ？” 并尝试使用分析仪的免费试用版。

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.

这次，我将看一下PMDK项目，并向您介绍我注意到的错误和缺点。 我内心的感觉告诉我，它们并不多，这表明项目代码的质量很高。 至于一些奇特的事情，我发现了一些不正确的代码片段，尽管它们仍然可以正常工作:)。 我的意思将从故事的其余部分变得更加清楚。

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. The source code is available here: pmdk.

因此，PMDK是开放源代码库和工具的集合，旨在简化支持NVRAM的应用程序的开发，调试和管理。 在此处查看更多详细信息： PMDK简介 。 源代码在这里： pmdk 。

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 :).

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

无效的代码有效 (Incorrect code that works )

分配的内存大小 (Size of allocated memory)

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.

当程序无法正常工作时，程序员通常会花时间调试代码。 但是，有时在某些情况下程序可以正常运行，但是代码中包含错误。 程序员只是很幸运，错误并没有显示出来。 在PMDK项目中，我偶然发现了几个此类有趣的案例，因此我决定将它们放在单独的部分中。

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

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

A classic typo due to which the wrong amount of memory is allocated. The sizeof operator will return the size of the pointer to the structure instead of the size of this structure. The correct version is:

由于分配错误的内存而造成的经典错字。 sizeof运算符将返回指向该结构的指针的大小，而不是该结构的大小。 正确的版本是：

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

or

要么

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

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

但是，此错误编写的代码可以正常工作。 事实是池结构仅包含一个指针：

struct pool {
  struct ctl *ctl;
};

It turns out that the structure takes exactly as much space as the pointer. So that's all right.

事实证明，该结构占用的空间恰好等于指针的空间。 没关系。

弦长 (String length)

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

让我们继续下一种情况，其中再次使用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 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

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

To copy a string, a pointer to a special copy function is used. Note the call to this function, or rather its third argument.

要复制字符串，使用指向特殊复制功能的指针。 请注意对该函数的调用，或者是它的第三个参数。

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

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

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.

幸运的是，字符串由8个字符组成，如果正在构建64位应用程序，则其大小与指针的大小匹配。 结果，字符串“ No code”的所有8个字符。 将被成功复制。

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.

实际上，情况更加复杂和令人着迷。 该错误的解释取决于作者是否要复制终端null。 让我们考虑两种情况。

场景1 (Scenario 1.)

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

终端null必须复制。 这样，我错了，这不仅不是无害的错误，而且不会自我显现。 仅复制了8个字节，而不是9个字节。 没有终端空值，并且无法预测后果。 在这种情况下，可以通过更改

initial_state constant string as follows: initial_state常量字符串如下：

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

Now the value of sizeof(initial_state) is 9.

现在sizeof(initial_state)的值为9。

场景2。 (Scenario 2.)

Terminal null is not required at all. For example, you can see this line of code below:

根本不需要终端null。 例如，您可以在下面看到以下代码行：

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

As you can see, the strlen function returns 8 and terminal null is not involved in the comparison. Then it's really good luck and all is well.

如您所见， strlen函数返回8，并且比较中不包含终端null。 那真是好运，一切都很好。

按位移位 (Bitwise shift)

The following example is related to the 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

PVS-Studio警告：V610 [CWE-758]未指定的行为。 检查移位运算符“ >>”。 左操作数“〜0”为负。 克隆cpp 205

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.

将负值向右移的结果取决于编译器的实现。 因此，尽管此代码可以在所有当前现有的应用程序编译模式下正确且预期地工作，但它仍然是一个运气。

操作优先 (Operation precedence)

And let's look at the last case related to the operation precedence.

让我们看一下与操作优先级有关的最后一种情况。

#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

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

To get a constant equal to 20 MB, the programmer decided to follow these steps:

为了获得等于20 MB的常数，程序员决定遵循以下步骤：

• Shifted 1 by 20 bits to get the value 1048576, i.e. 1 MB.
  将1移位20位以获得值1048576，即1 MB。
• Multiplied 1 MB by 20.
  1 MB乘以20。

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

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

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

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

Agree it is unlikely that the programmer wanted the expression to be calculated in such a sequence. There is no point in multiplying 20 by 1. So this is the case where the code doesn't work the way the programmer intended.

同意程序员不太可能希望以此顺序计算表达式。 将20乘以1是没有意义的。因此，在这种情况下，代码无法按照程序员的预期工作。

But this error won't manifest itself in any way. It doesn't matter how to write it:

但是此错误不会以任何方式显现出来。 没关系写它：

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

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

结果总是一样的 ！ 始终获得期望值20971520，并且程序可以正常正确运行。

其他错误 (Other errors)

括号放在错误的位置 (Parentheses 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 follows: 'A = (B == C)'. ut.c 641

PVS-Studio警告：V593 [CWE-783]考虑查看“ A = B == C”类型的表达式。 该表达式的计算如下：“ A =(B == C)”。 ut.c 641

Take a careful look here:

在这里仔细看一下：

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

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

程序员希望将NtQuerySystemInformation函数返回的值存储在状态变量中，然后将其与常量进行比较。

The programmer probably knew that the priority of the comparison operator (==) is higher than that of the assignment operator (=), and therefore parentheses should be used. But probably made a mistake 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. After all, the status variable always contains the result of comparison, i.e. false (0) or true (1). So the condition ([0..1] >= 0) is always true.

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

潜在的内存泄漏 (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

PVS-Studio警告：V773 [CWE-401]在不释放“输入”指针的情况下退出了该功能。 可能发生内存泄漏。 第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 is best to postpone the check until memory is allocated:

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

Or one can explicitly free up 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

PVS-Studio警告：V1028 [CWE-190]可能的溢出。 考虑转换操作数，而不是结果。 memcpy_common.c 62

Explicit casting the addition result to the os_off_t type doesn't make sense. First, this doesn't protect against the potential overflow that can occur when two int values are added together. Second, the result of addition would have been perfectly extended to the os_off_t type implicitly. Explicit type casting is simply redundant.

将加法结果显式转换为os_off_t类型没有意义。 首先，这不能防止两个int值加在一起时可能发生的溢出。 第二，相加的结果将被隐式扩展为os_off_t类型。 显式类型转换只是多余的。

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

我认为这样写会更正确：

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

Here an unsigned value of the size_t type is converted to a signed value (to avoid a warning from the compiler). At the same time, overflow won't occur when adding.

在这里，将size_t类型的无符号值转换为有符号值(以避免编译器发出警告)。 同时，添加时不会发生溢出。

错误的防溢出保护 (Incorrect protection against overflow)

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

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

It's not very clear to me what is this case which the check should protect us from. Anyway, the check doesn't work. The rel_wait variable is of the DWORD unsigned type. This means that rel_wait < 0 doesn't make sense, since the result is always true.

我不清楚这是什么情况，支票应保护我们免受这种情况的侵害。 无论如何，检查不起作用。 rel_wait变量为DWORD无符号类型。 这意味着rel_wait <0没有意义，因为结果始终为true。

缺少检查是否已成功分配内存 (Missing check that memory was successfully allocated)

Checking that memory is allocated is performed 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 when malloc calls return NULL. Example:

使用断言宏执行检查是否已分配内存，如果编译了应用程序的发行版，则该宏不执行任何操作。 因此，可以说，当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 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

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

There is even no assert in other places:

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

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

PVS-Studio警告：V522 [CWE-690]可能会取消引用潜在的空指针“工人”。 检查线：126，124。pi.c 126

I counted at least 37 of such code fragments. So I don't see the point in listing all of them in the article.

我算出了至少37个这样的代码片段。 所以我看不出在文章中列出所有这些内容的意义。

At a first glance, the lack of checks can be considered self-indulgence and smelly code. I don't go along with this point of view. Programmers underestimate the danger of missing such checks. A null pointer won't necessarily immediately manifest itself as a crash when dereferencing. The consequences can be more bizarre and dangerous, especially in multithreaded programs. To understand more about what is happening and why checks are needed, I strongly recommend that everyone read the article "Why it is important to check what the malloc function returned".

乍一看，缺乏检查可以被认为是自我放纵和臭气熏天的代码。 我不同意这种观点。 程序员低估了丢失此类支票的危险。 在取消引用时，空指针不一定会立即显示为崩溃。 结果可能会更加奇怪和危险，尤其是在多线程程序中。 要了解有关正在发生的事情以及为什么需要检查的更多信息，我强烈建议每个人都阅读文章“ 检查malloc函数返回的内容为什么很重要 ”。

代码气味 (Code smell)

两次调用CloseHandle (Double call of 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

PVS-Studio警告：V586 [CWE-675]两次调用“ CloseHandle”功能以释放同一资源。 pmem2_map.c 76

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

查看此代码和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)

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

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

I'm not going to clutter the article with description of each macro and torture a reader by forcing to nest one macro into another in their head. Let's look at the final version of the expanded code 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 delete the always false condition 0 && 0) and every part that's irrelevant. Here's what we get:

让我们删除始终为假的条件0 && 0)和所有不相关的部分。 这是我们得到的：

((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 debugging message is generated and CloseHandle is called for the same incorrect handle to get the error code again.

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

There seems to be no mistake. Once the handle is invalid, it's okay that the CloseHandle function is called twice for it. However, this code has a smell, indeed. It would be more ideologically correct to call the function only once and save the status that it returned, so that if necessary, it can display its value in the message.

似乎没有错。 一旦句柄无效，可以为它两次调用CloseHandle函数。 但是，此代码确实有气味。 从意识形态上讲，只调用一次该函数并保存其返回的状态在思想上是正确的，以便在必要时可以在消息中显示其值。

实现的接口之间不匹配(常量丢失) (The mismatch between the interface of the implementation (constness dropping))

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 issues the message: V530 [CWE-252] The return value of function 'status_msg_info_and_question' is required to be utilized. check_util.c 293

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

The reason is that the status_msg_info_and_question function, from the analyzer's point of view, doesn't change the state of objects external to it, including the passed constant string. In other words, the function just counts something and returns the result. And if so, it is strange not to use the result that this function returns. Although the analyzer is wrong this time, it points to the code smell. Let's see how the called status_msg_info_and_question function works.

原因是，从分析器的角度来看， 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;
}

When calling the strchr function, constness is implicitly cast away. The fact is that in C it is declared as follows:

调用strchr函数时，会隐式地丢弃constness 。 事实是在C中它声明如下：

char * strchr ( const char *, int );

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

不是最好的解决方案。 但是C语言就是这样：)。

The analyzer got confused and didn't get that the passed string was actually being changed. If this is the case, then the return value is not the most important one and you don't need to use it.

分析器感到困惑，并且没有获得所传递的字符串实际被更改的信息。 如果是这种情况，则返回值不是最重要的值，您不需要使用它。

However, even though the analyzer got confused, it points to a code smell. What confuses the analyzer can also confuse the person who maintains the code. It would be better to declare the function more honestly by removing const:

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

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

This way the intent is immediately clear, 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

PVS-Studio警告：V547 [CWE-571]表达式'blocks [i]'始终为true。 第1054章

If blocks[i] == NULL, the continue statement executes and the loop starts the next iteration. Therefore, rechecking the blocks[i]] element doesn't make sense and the ternary operator is unnecessary. The code can be simplified:

如果blocks [i] == NULL ，则执行继续语句，然后循环开始下一次迭代。 因此，重新检查blocks [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;
}
....

可疑使用空指针 (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

PVS-Studio警告：V1004 [CWE-119]在针对nullptr进行验证之后，不安全地使用了'(char *)mt-> BaseAddress'指针。 检查行：226，235。win_mmap.c 235

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

mt-> BaseAddress指针可以为null，如检查所示：

if (mt->BaseAddress != NULL)

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

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

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

Some large integer value will be obtained, which is actually equal to the value of the mt->EndAddress pointer. This may not be an error, but it looks very suspicious, and I think the code should be rechecked. The code smells as it is incomprehensible and it clearly lacks explanatory comments.

将获得一些大的整数值，该值实际上等于mt-> EndAddress指针的值。 这可能不是一个错误，但是看起来非常可疑，我认为应该重新检查代码。 该代码闻起来令人难以理解，并且显然缺少解释性注释。

全局变量的简称 (Short names of global variables)

I believe that the code smells if it contains global variables with short names. It is easy to mistype and accidentally use a global variable in some function instead of a local one. Example:

我相信，如果代码包含带有短名称的全局变量，该代码就会闻起来。 很容易输入错误，并且在某些函数中意外地使用了全局变量而不是局部变量。 例：

static struct critnib *c;

PVS-Studio warnings for such variables:

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

• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'ri' variable. map.c 131
  V707为全局变量使用短名称被认为是不好的做法。 建议重命名“ ri”变量。 第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为全局变量使用短名称被认为是不好的做法。 建议重命名“ c”变量。 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为全局变量使用短名称被认为是不好的做法。 建议重命名“ Id”变量。 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
  V707为全局变量使用短名称被认为是不好的做法。 建议重命名“ Id”变量。 obj_list.c 34

陌生的东西 (Stranger things)

As for me, the do_memmove function contained the weirdest code. The analyzer issued two warnings that indicate either very serious errors, or the fact that I simply don't understand what was meant. Since the code is very peculiar, I decided to review the warnings issued in a separate section of the article. So, the first warning is issued here.

对于我来说， 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 warning: V549 [CWE-688] The first argument of 'memmove' function is equal to the second argument. memmove_common.c 71

PVS-Studio警告：V549 [CWE-688]“ memmove”功能的第一个参数等于第二个参数。 memmove_common.c 71

Note that the first and second arguments of the function are the same. So the function doesn't actually do anything. What options come to mind:

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

• The author wanted to "touch" the memory block. But will this happen in reality? Will the optimizing compiler remove the code that copies a block of memory to itself?
  作者想“触摸”内存块。 但这会在现实中发生吗？ 优化的编译器是否会删除将一块内存复制到自身的代码？

• This is some kind of a unit test for the memmove function.

  这是对memmove函数的某种单元测试。

• The code contains a typo.
  该代码包含一个错字。

And here is an equally strange fragment 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

PVS-Studio警告：V575 [CWE-628]“记忆”功能处理“ 0”元素。 检查第三个论点。 memmove_common.c 82

The function transfers 0 bytes. What's that – an error or just an extra check? A unit test? A typo?

该函数传输0字节。 那是什么–错误或只是额外的检查？ 单元测试？ 错字了吗

For me, this code is incomprehensible and strange.

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

为什么要使用代码分析器？ (Why use code analyzers?)

It may seem that since few errors are found, the introducing an analyzer in the code development process is not justified. But the point of using static analysis tools is not to perform one-time checks, but to regularly detect errors at the code writing stage. Otherwise, these errors are detected in more expensive and slower ways (debugging, testing, user feedback, and so on). This idea is described in more detail in the article "Errors that static code analysis does not find because it is not used", which I recommend getting acquainted with. And feel free to visit our website to download and try PVS-Studio to scan your projects.

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

Thanks for your attention!

感谢您的关注！

翻译自: https://habr.com/en/company/pvs-studio/blog/515730/