# Why SSDLC needs static analysis: a case study of 190 bugs in TDengine

Static code analysis is one of the most important components of secure software development\. It detects errors and potential vulnerabilities early in the development process, when they're cheaper and easier to fix\. It also enables developers to detect security issues and flaws that they aren't aware of\.

![1249_TDengine_4/image1.png](https://import.viva64.com/docx/blog/1249_TDengine_4/image1.png)

This article provides examples to show why static analysis is important, rather than simply speculates on the idea of using analyzers for safety\. So, let's take a practical look at how static analysis can make your code safer, more reliable, and neater\.

We'll continue examining the [TDengine](https://github.com/taosdata/TDengine) project, which we've covered in three small notes on code refactoring:

1. Breaking down bugs in TDengine to master refactoring, part 1: [sausage code](https://pvs-studio.com/en/blog/posts/cpp/1230/)\.
1. Breaking down bugs in TDengine to master refactoring, part 2: [stack\-consuming macro](https://pvs-studio.com/en/blog/posts/cpp/1238/)\.
1. Breaking down bugs in TDengine to master refactoring, part 3: [price of laziness](https://pvs-studio.com/en/blog/posts/cpp/1242/)\.

TDengine is a database designed for IoT systems, where reliability and security are especially critical, which makes this project stand\-out more\. Checking the code using PVS\-Studio static analyzer was especially interesting since it detects not only typos but also potential vulnerabilities\.


> TDengine is an open source, high\\\-performance, cloud native time\\\-series database optimized for Internet of Things \\\(IoT\\\), Connected Cars, and Industrial IoT\\\. It enables efficient, real\\\-time data ingestion, processing, and monitoring of TB and even PB scale data per day, generated by billions of sensors and data collectors\\\.

The fourth part of the series is released much long after the project check\. So, some code snippets may look different now, and some errors may already be fixed\. However, it's not a big deal—this article is intended to highlight the value of static analysis as a practice, rather than find and fix as many errors as possible\. Such one\-time checks show the capabilities of PVS\-Studio analyzer but they don't contribute meaningfully to long\-term quality and reliability of the project\. Static analysis should be used regularly\. [Introduce Static Analysis in the Process, Don't Just Search for Bugs with It](https://habr.com/en/articles/440610/)\.

Usually we only check the project code, discarding third\-party libraries that it uses\. However, this time I deliberately checked the entire codebase with external libraries\. I'd like you to think over the following take:


> From the user's point of view, it doesn't matter whether a bug or vulnerability originates in our own code or in the code of a library we use\\\. If we use third\\\-party code, we take responsibility for it\\\. Errors and issues in third\\\-party code become ours\\\.

It is useful, and sometimes necessary, to perform static analysis of the third\-party components that we use:

1. This will [help us choose](https://pvs-studio.com/en/blog/posts/cpp/0762/) safer and more reliable libraries for future use;
1. We can proactively find and eliminate [zero\-day vulnerabilities](https://pvs-studio.com/en/blog/terms/6478/) that might affect the product reputation\. Errors from third\-party components don't seem to be our fault, but this fact doesn't make it any easier\.
1. By fixing bugs in third\-party libraries, we contribute to the development of open\-source code\.

## 130 shades of null pointers

Dereferencing a null pointer is a very frequent error\. In this regard, the TDengine project code is no exception\.

### Error N1\. Selection error

The `taosArrayGetLast` function can return `NULL`:

```cpp
void* taosArrayGetLast(const SArray* pArray) {
  if (pArray->size == 0) {
    terrno = TSDB_CODE_INVALID_PARA;
    return NULL;
  }

  return TARRAY_GET_ELEM(pArray, pArray->size - 1);
}
```

The author of the following code attempted to handle this scenario, but failed\.

```cpp
static int32_t walInitWriteFile(SWal *pWal) {
  int64_t       fileFirstVer = -1;
  ....
  SWalFileInfo *pRet = taosArrayGetLast(pWal->fileInfoSet);
  if (pRet == NULL) {
    fileFirstVer = pWal->vers.lastVer + 1;
  }
  fileFirstVer = pRet->firstVer;
  ....
}
```

Regardless of the `pRet` pointer value, the pointer is still dereferenced\. In addition, the value of the `fileFirstVer` variable is always overwritten\. So, the analyzer issues two warnings at once:

* V519 The 'fileFirstVer' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 696, 698\. walWrite\.c 698
* V1004 The 'pRet' pointer was used unsafely after it was verified against nullptr\. Check lines: 695, 698\. walWrite\.c 698

Perhaps the developer should have added `else` to remedy the situation: 

```cpp
SWalFileInfo *pRet = taosArrayGetLast(pWal->fileInfoSet);
if (pRet == NULL) {
  fileFirstVer = pWal->vers.lastVer + 1;
} else {
  fileFirstVer = pRet->firstVer;
}
```

### Errors N2–N6\. Errors in error handlers

Bugs often turn up in error handlers, and null pointer dereferences are no exception\. It's hardly surprising since almost no one ever tests these parts of code\. Neither does anyone write unit tests for them—it's just too tedious\.

```cpp
int32_t ctgGetFetchName(SArray* pNames, SCtgFetch* pFetch, SName** ppName) {
  STablesReq* pReq = (STablesReq*)taosArrayGet(pNames, pFetch->dbIdx);
  if (NULL == pReq) {
    qError("fail to get the %dth tb in pTables, tbNum:%d",
           pFetch->tbIdx, (int32_t)taosArrayGetSize(pReq->pTables));
    return TSDB_CODE_CTG_INTERNAL_ERROR;
  }
  ....
}
```

The PVS\-Studio warning: [V522](https://pvs-studio.com/en/docs/warnings/v522/) Dereferencing of the null pointer 'pReq' might take place\. ctgUtil\.c 1769

If the `pReq` pointer is null, it's dereferenced to print the number of elements in the table:

```cpp
STablesReq* pReq = ;
if (NULL == pReq) {
  ....(pReq->pTables));
```

A dubious idea :\)

On the one hand, the error doesn't seem crucial: it's unlikely to come up, otherwise it would have been noticed and fixed\.

On the other hand, it's critical:

1. In case of a failure, the program will crash instead of providing a reasonable message to identify and fix the error\. Perhaps, users have complained about such crashes before, but the developers might not have realized that the bug lies here\. We're expecting a message \.\.\. but there isn't one :\)
1. Dereferencing a null pointer makes behavior undefined\. An optimizing compiler can do anything with this code\. For example, it can delete the check and the message printing altogether, assuming that a pointer can't be null :\)

Here are similar defects in error handlers:

1. V522 Dereferencing of the null pointer 'pBufInfo' might take place\. groupcacheoperator\.c 391
1. V522 Dereferencing of the null pointer 'item' might take place\. scanoperator\.c 4756
1. V522 Dereferencing of the null pointer 'pTrans' might take place\. mndCompact\.c 710
1. V522 Dereferencing of the null pointer 'pEntry' might take place\. syncPipeline\.c 885

### Errors N7–N10\. Incorrect assert usage

```cpp
template <typename It>
static void
linkResultDirectedEdges(It first, It last)
// throw(TopologyException);
{
  for(; first != last; ++first) {
    Node* node = *first;
    assert(node);

    EdgeEndStar* ees = node->getEdges();
    assert(ees);
    DirectedEdgeStar* des = dynamic_cast<DirectedEdgeStar*>(ees);
    assert(des);

    // this might throw an exception
    des->linkResultDirectedEdges();
  }
}
```

The PVS\-Studio warning: [V522](https://pvs-studio.com/en/docs/warnings/v522/) There might be dereferencing of a potential null pointer 'des'\. PlanarGraph\.h 98

The `dynamic_cast` operator may return a null pointer, and therefore must be checked\. However, using `assert` is clearly incorrect in this case\. Such a check is of little use\. If the pointer is null when running the debug version, the error will be noticed even without the `assert` operator\. In the case of a release build, the `assert` macro will turn into nothing, and the use of a null pointer, leading to undefined behavior in the future\.

Assertions \(`assert`\) are intended to ensure that the data is within the expected ranges while testing the application\. But in this case, by using `dynamic_cast`, the programmer implies that the object casting may fail and the pointer will be null\. In other words, a null pointer is an expected option\. If a developer wants type casting to always perform successfully, they should have used `static_cast`\. Don't hesitate to read the article on a related topic: "[Why it is bad idea to check result of malloc call with assert](https://pvs-studio.com/en/blog/posts/cpp/1104/)"\.

It's better to replace `assert` with the `if` operator and write the code that handles the case when the pointer is null\.

These are other similar errors:

1. V522 There might be dereferencing of a potential null pointer 'nextedge'\. LineMergeDirectedEdge\.cpp 64
1. V522 There might be dereferencing of a potential null pointer 'edge'\. EdgeRing\.cpp 225
1. V522 There might be dereferencing of a potential null pointer 'point'\. PointGeometryUnion\.cpp 52

### Errors N11–N13\. dynamic\_cast gets bolder

There is no check at all after `dynamic_cast` is executed\. A null pointer can be dereferenced while evaluating a condition when `nextedge->getEdgeDirection()` is called\.

```cpp
LineMergeDirectedEdge*
LineMergeDirectedEdge::getNext(bool checkDirection)
{
  ....
  if(getToNode()->getOutEdges()->getEdges()[0] == getSym()) {
    auto nextedge = dynamic_cast<LineMergeDirectedEdge*>(
      getToNode()->getOutEdges()->getEdges()[1]);
    return (!checkDirection || nextedge->getEdgeDirection()) ?
      nextedge : nullptr;
  }
  ....
}
```

The PVS\-Studio warning: [V522](https://pvs-studio.com/en/docs/warnings/v522/) There might be dereferencing of a potential null pointer 'nextedge'\. LineMergeDirectedEdge\.cpp 57

The result of `dynamic_cast` must be checked\. If the type casting is expected to succeed, the faster `static_cast` operator should be used instead\. This will add clarity to those who maintain the code\.

In this case, it seems logical to me to refine the condition:

```cpp
auto nextedge = dynamic_cast<LineMergeDirectedEdge*>(
  getToNode()->getOutEdges()->getEdges()[1]);
return (!checkDirection ||
        (nextedge && nextedge->getEdgeDirection())) ?
  nextedge : nullptr;
```

However, this code looks complicated, so let's make it more readable:

```cpp
auto nextedge = dynamic_cast<LineMergeDirectedEdge*>(
  getToNode()->getOutEdges()->getEdges()[1]);

if (!checkDirection ||
    (nextedge && nextedge->getEdgeDirection()))
{
  return nextedge;
}
return nullptr;
```

Other warnings:

1. V522 There might be dereferencing of a potential null pointer\. EdgeRing\.cpp 300
1. V522 There might be dereferencing of a potential null pointer\. EdgeRing\.cpp 318

### Error N14\. Macros\.\.\.

Do you remember the article about a [stack\-consuming macro](https://pvs-studio.com/en/blog/posts/cpp/1238/)? It said that macros can harbor unpleasant surprises that are hard to notice on a code review\. Here comes another example of a "macro mess"\.

```cpp
int32_t qWorkerInit(....) {
  ....
  if (NULL == mgmt->schHash) {
    taosMemoryFreeClear(mgmt);
    qError("init %d scheduler hash failed", mgmt->cfg.maxSchedulerNum);
    QW_ERR_JRET(terrno);
  }
  ....
}
```

Did you spot the error? I guess not\. Reviewing this code, one may find it hard to see what's wrong here\.

The issue is that `taosMemoryFreeClear` is not a function call to free memory, but a macro that also nullifies the pointer\.

```cpp
#define taosMemoryFreeClear(ptr)   \
  do {                             \
    if (ptr) {                     \
      taosMemoryFree((void *)ptr); \
      (ptr) = NULL;                \
    }                              \
  } while (0)
```

So, an attempt to print a message will result in dereferencing a null pointer\.

The PVS\-Studio warning: [V522](https://pvs-studio.com/en/docs/warnings/v522/) Dereferencing of the null pointer 'mgmt' might take place\. qworker\.c 1442

To fix the code, the author should place the `qError` function call before the macro\.

```cpp
if (NULL == mgmt->schHash) {
  qError("init %d scheduler hash failed", mgmt->cfg.maxSchedulerNum);
  taosMemoryFreeClear(mgmt);
  QW_ERR_JRET(terrno);
}
```

Oh, those macros\.\.\.

### Errors N15–N112\. No checks when allocating memory

The TDengine project devs should keep an eye on memory allocation checks after _malloc_ functions \(or similar ones\)\. Sometimes the checks are there:

```cpp
void* buf = taosMemoryMalloc(tlen);
if (NULL == buf) {
  taosArrayDestroy(reqNew.pArray);
  tDeleteSVCreateTbBatchReq(&req);
  goto end;
}
```

But often, there are none\. This is quite sad and bad given it's a library for IoT devices:

1. Ignoring memory allocation errors is generally a no\-go for libraries\. We can't predict how and where the library will be used\. So, if something goes wrong, the library authors must notify the app devs so they could handle a situation\.
1. IoT often involves embedded devices with a relatively limited memory, where memory shortages are not such an exotic situation to be handled\.
1. Just imagine this awkward moment—an app randomly crashes because the library developer didn't account for memory issues\. Worse, this may lead to database inconsistency\.

Learn more details in the article: "[Four reasons to check what the malloc function returned](https://pvs-studio.com/en/blog/posts/cpp/0938/)"\. If someone in your team doesn't check pointers after `malloc`, I suggest you gently make them read this article—repeatedly—until they are fully aware and enlightened\.

What does the lack of checks look like in TDengine? Diverse, yet boring to delve into\. I'm going to give you a couple examples\.

```cpp
taos_linked_list_t *taos_linked_list_new(void) {
  taos_linked_list_t *self =
    (taos_linked_list_t *)taos_malloc(sizeof(taos_linked_list_t));
  self->head = NULL;
  self->tail = NULL;
  self->free_fn = NULL;
  self->compare_fn = NULL;
  self->size = 0;
  return self;
}
```

The PVS\-Studio warning: [V522](https://pvs-studio.com/en/docs/warnings/v522/) There might be dereferencing of a potential null pointer 'self'\. Check lines: 28, 27\. taos\_linked\_list\.c 28

```cpp
unsigned char*
SZ_skip_compress_double(double* data, size_t dataLength, size_t* outSize)
{
  *outSize = dataLength*sizeof(double);
  unsigned char* out = (unsigned char*)malloc(dataLength*sizeof(double));
  memcpy(out, data, dataLength*sizeof(double));
  return out;
}
```

The PVS\-Studio warning: [V575](https://pvs-studio.com/en/docs/warnings/v575/) The potential null pointer is passed into 'memcpy' function\. Inspect the first argument\. Check lines: 28, 27\. sz\_double\.c 28

<details>
   <summary>Other similar warnings\\\.</summary>

1. V522 There might be dereferencing of a potential null pointer '\* coeff\_array'\. Check lines: 304, 303\. dataCompression\.c 304
1. V522 There might be dereferencing of a potential null pointer 'keys'\. Check lines: 344, 333\. iniparser\.c 344
1. V522 Dereferencing of the null pointer 'vce' might take place\. The potential null pointer is passed into 'compressSingleFloatValue' function\. Inspect the first argument\. Check lines: 209, 439, 433\. dataCompression\.c 209
1. V522 Dereferencing of the null pointer 'lce' might take place\. The potential null pointer is passed into 'addExactData' function\. Inspect the fourth argument\. Check lines: 275, 442, 434\. dataCompression\.c 275
1. V522 There might be dereferencing of a potential null pointer '\* decData'\. Check lines: 514, 463\. dataCompression\.c 514
1. V522 Dereferencing of the null pointer 'vce' might take place\. The potential null pointer is passed into 'compressSingleDoubleValue' function\. Inspect the first argument\. Check lines: 234, 575, 569\. dataCompression\.c 234
1. V522 Dereferencing of the null pointer 'lce' might take place\. The potential null pointer is passed into 'addExactData' function\. Inspect the fourth argument\. Check lines: 275, 578, 570\. dataCompression\.c 275
1. V522 There might be dereferencing of a potential null pointer 'result'\. Check lines: 143, 134\. CompressElement\.c 143
1. V522 There might be dereferencing of a potential null pointer 'type'\. Check lines: 152, 122\. szd\_double\.c 152
1. V522 There might be dereferencing of a potential null pointer '\* dia'\. Check lines: 18, 17\. DynamicIntArray\.c 18
1. V522 There might be dereferencing of a potential null pointer 'type'\. Check lines: 130, 107\. sz\_double\.c 130
1. V522 There might be dereferencing of a potential null pointer 'vce'\. Check lines: 132, 126\. sz\_double\.c 132
1. V522 There might be dereferencing of a potential null pointer 'types'\. Check lines: 160, 129\. szd\_float\.c 160
1. V522 There might be dereferencing of a potential null pointer 'type2code'\. Check lines: 48, 43\. transcode\.c 48
1. V522 There might be dereferencing of a potential null pointer 'diff'\. Check lines: 49, 44\. transcode\.c 49
1. V522 There might be dereferencing of a potential null pointer 'tp\_code'\. Check lines: 63, 36\. transcode\.c 63
1. V522 There might be dereferencing of a potential null pointer 'tp\_code'\. Check lines: 146, 106\. transcode\.c 146
1. V522 There might be dereferencing of a potential null pointer 'type'\. Check lines: 138, 114\. sz\_float\.c 138
1. V522 There might be dereferencing of a potential null pointer 'vce'\. Check lines: 141, 134\. sz\_float\.c 141
1. V522 There might be dereferencing of a potential null pointer '\* dba'\. Check lines: 18, 17\. DynamicByteArray\.c 18
1. V522 There might be dereferencing of a potential null pointer 'huffmanTree\-\>code\[n\-\>c\]'\. Check lines: 129, 125\. Huffman\.c 129
1. V522 There might be dereferencing of a potential null pointer '\* out'\. Check lines: 425, 424\. Huffman\.c 425
1. V522 There might be dereferencing of a potential null pointer 'symbol'\. Check lines: 633, 632\. dumper\.c 633
1. V522 There might be dereferencing of a potential null pointer 'stackTrace'\. Check lines: 716, 715\. dumper\.c 716
1. V522 There might be dereferencing of a potential null pointer 'subgeomArray'\. Check lines: 2084, 2082\. geos\_ts\_c\.cpp 2084
1. V522 There might be dereferencing of a potential null pointer '\* vgroup\_ids'\. Check lines: 97, 83\. taos\_counter\.c 97
1. V522 There might be dereferencing of a potential null pointer '\* keys'\. Check lines: 98, 88\. taos\_counter\.c 98
1. V522 There might be dereferencing of a potential null pointer 'node'\. Check lines: 92, 90\. taos\_linked\_list\.c 92
1. V522 There might be dereferencing of a potential null pointer 'node'\. Check lines: 108, 106\. taos\_linked\_list\.c 108
1. V522 There might be dereferencing of a potential null pointer 'self'\. Check lines: 42, 41\. taos\_map\.c 42
1. V522 There might be dereferencing of a potential null pointer 'self'\. Check lines: 78, 77\. taos\_map\.c 78
1. V522 There might be dereferencing of a potential null pointer 'self\-\>addrs'\. Check lines: 98, 94\. taos\_map\.c 98
1. V522 There might be dereferencing of a potential null pointer 'new\_addrs'\. Check lines: 287, 283\. taos\_map\.c 287
1. V522 There might be dereferencing of a potential null pointer 'self'\. Check lines: 35, 34\. taos\_metric\_formatter\.c 35
1. V522 There might be dereferencing of a potential null pointer 'k'\. Check lines: 60, 47\. taos\_metric\.c 60
1. V522 There might be dereferencing of a potential null pointer 'self'\. Check lines: 45, 44\. taos\_string\_builder\.c 45
1. V522 There might be dereferencing of a potential null pointer 'self\-\>str'\. Check lines: 59, 58\. taos\_string\_builder\.c 59
1. V522 There might be dereferencing of a potential null pointer 'self'\. Check lines: 49, 47\. taos\_collector\_registry\.c 49
1. V522 There might be dereferencing of a potential null pointer 'self'\. Check lines: 39, 38\. taos\_metric\_sample\.c 39
1. V522 There might be dereferencing of a potential null pointer 'e'\. Check lines: 532, 530\. lru\_cache\.cc 532
1. V522 There might be dereferencing of a potential null pointer 'column\_families'\. Check lines: 1038, 1036\. c\.cc 1038
1. V522 There might be dereferencing of a potential null pointer 'cf\_names'\. Check lines: 2526, 2522\. c\.cc 2526
1. V522 There might be dereferencing of a potential null pointer 'cf\_options'\. Check lines: 2527, 2523\. c\.cc 2527
1. V522 There might be dereferencing of a potential null pointer 'level\_meta'\. Check lines: 5308, 5307\. c\.cc 5308
1. V522 There might be dereferencing of a potential null pointer 'file\_meta'\. Check lines: 5339, 5338\. c\.cc 5339
1. V522 There might be dereferencing of a potential null pointer 'buf'\. Check lines: 5599, 5596\. c\.cc 5599
1. V522 There might be dereferencing of a potential null pointer 'wi'\. Check lines: 5627, 5626\. c\.cc 5627
1. V522 There might be dereferencing of a potential null pointer 'result'\. Check lines: 5672, 5671\. c\.cc 5672
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 51, 50\. sz\_double\.c 51
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 273, 272\. sz\_double\.c 273
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 380, 379\. sz\_double\.c 380
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 47, 46\. sz\_float\.c 47
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 275, 274\. sz\_float\.c 275
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 402, 401\. sz\_float\.c 402
1. V575 The potential null pointer is passed into 'memcpy' function\. Inspect the first argument\. Check lines: 33, 27\. DynamicByteArray\.c 33
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 20, 19\. Huffman\.c 20
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 29, 24\. Huffman\.c 29
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 30, 25\. Huffman\.c 30
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 31, 26\. Huffman\.c 31
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 32, 27\. Huffman\.c 32
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 172, 171\. Huffman\.c 172
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 194, 193\. Huffman\.c 194
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 413, 412\. Huffman\.c 413
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 415, 414\. Huffman\.c 415
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 417, 416\. Huffman\.c 417
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 419, 418\. Huffman\.c 419
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 440, 439\. Huffman\.c 440
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 442, 441\. Huffman\.c 442
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 444, 443\. Huffman\.c 444
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 446, 445\. Huffman\.c 446
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 464, 463\. Huffman\.c 464
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 466, 465\. Huffman\.c 466
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 468, 467\. Huffman\.c 468
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 470, 469\. Huffman\.c 470
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 566, 565\. Huffman\.c 566
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 568, 567\. Huffman\.c 568
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 570, 569\. Huffman\.c 570
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 572, 571\. Huffman\.c 572
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 603, 602\. Huffman\.c 603
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 605, 604\. Huffman\.c 605
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 607, 606\. Huffman\.c 607
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 609, 608\. Huffman\.c 609
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 656, 655\. Huffman\.c 656
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 658, 657\. Huffman\.c 658
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 660, 659\. Huffman\.c 660
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 662, 661\. Huffman\.c 662
1. V575 The potential null pointer is passed into 'memcpy' function\. Inspect the first argument\. Check lines: 710, 708\. Huffman\.c 710
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 22, 21\. TightDataPointStorageF\.c 22
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 194, 193\. TightDataPointStorageF\.c 194
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 22, 21\. TightDataPointStorageD\.c 22
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 194, 193\. TightDataPointStorageD\.c 194
1. V575 The potential null pointer is passed into 'memcpy' function\. Inspect the first argument\. Check lines: 3320, 3319\. geos\_ts\_c\.cpp 3320
1. V575 The potential null pointer is passed into 'memcpy' function\. Inspect the first argument\. Check lines: 3337, 3336\. geos\_ts\_c\.cpp 3337
1. V575 The potential null pointer is passed into 'memset' function\. Inspect the first argument\. Check lines: 42, 41\. taos\_metric\.c 42
1. V575 The potential null pointer is passed into 'memcpy' function\. Inspect the first argument\. Check lines: 145, 144\. taos\_string\_builder\.c 145
1. V575 The potential null pointer is passed into 'memcpy' function\. Inspect the first argument\. Check lines: 533, 532\. c\.cc 533


</details>


**Note\.** Some may point out that some errors stem from libraries rather than TDengine itself\. But as I've already mentioned, when it comes to secure development, it doesn't matter where exactly the bug is found\. Let me repeat once again: there is no difference for a user whether the application crashes because of an error in TDengine or in another library that was needed to build TDengine\. Developers are responsible not only for the quality of their own code, but also for the quality of the third\-party code they use\.

### Errors N113–N130 \(in fact, more\)\. Pointer dereference before the check

It is a common error when a pointer is used before it's checked\. The simplest error of this type is as follows:

```cpp
bool
RectangleIntersection::clip_linestring_parts(
  const geom::LineString* gi, ....)
{
  auto n = gi->getNumPoints();

  if(gi == nullptr || n < 1) {
    return false;
  }
  ....
}
```

The PVS\-Studio warning: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'gi' pointer was utilized before it was verified against nullptr\. Check lines: 137, 139\. RectangleIntersection\.cpp 137

The `gi` pointer check should have been written earlier\. I think there is no point in elaborating on this case\.

Here is a similar case:

```cpp
int32_t tsortOpen(SSortHandle* pHandle) {
  int32_t code = 0;
  if (pHandle->opened) {
    return code;
  }

  if (pHandle == NULL || pHandle->fetchfp == NULL ||
      pHandle->comparFn == NULL) {
    return TSDB_CODE_INVALID_PARA;
  }
  ....
}
```

[V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'pHandle' pointer was utilized before it was verified against nullptr\. Check lines: 2883, 2887\. tsort\.c 2883

It's the same here\. There are some variations, but hopefully the point is clear\. If the logic behind the [V595](https://pvs-studio.com/en/docs/warnings/v595/) diagnostic rule doesn't seem quite obvious to you, you may learn more about it from this post: "[Explanation on Diagnostic V595](https://pvs-studio.com/en/docs/warnings/v595/)"\. It's been 10 years since this note was written\. Since then, its data flow analysis has grown more advanced, and it now evaluates possible pointer values with greater precision\. However, this does not make this diagnostic rule any less useful, and it still effectively finds bugs caused by incorrect ordering of pointer dereference and null checks\.

<details>
   <summary>Other errors of this type\\\.</summary>

1. V595 The 'col' pointer was utilized before it was verified against nullptr\. Check lines: 2075, 2076\. geos\_ts\_c\.cpp 2075
1. V595 The 'keys' pointer was utilized before it was verified against nullptr\. Check lines: 88, 89\. taos\_counter\.c 88
1. V595 The 'dbCache' pointer was utilized before it was verified against nullptr\. Check lines: 196, 199\. ctgCache\.c 196
1. V595 The 'pDbCache' pointer was utilized before it was verified against nullptr\. Check lines: 1631, 1633\. ctgCache\.c 1631
1. V595 The 'pInfo\-\>pState' pointer was utilized before it was verified against nullptr\. Check lines: 155, 158\. streamfilloperator\.c 155
1. V595 The 'pFillInfo' pointer was utilized before it was verified against nullptr\. Check lines: 1582, 1588\. streamfilloperator\.c 1582
1. V595 The 'string' pointer was utilized before it was verified against nullptr\. Check lines: 869, 870\. mndTopic\.c 869
1. V595 The 'pFile' pointer was utilized before it was verified against nullptr\. Check lines: 1351, 1353\. osFile\.c 1351
1. V595 The 'bins' pointer was utilized before it was verified against nullptr\. Check lines: 4267, 4268\. sclfunc\.c 4267
1. V595 The 'pCtx\-\>freeFunc' pointer was utilized before it was verified against nullptr\. Check lines: 351, 358\. schUtil\.c 351
1. V595 The 'pNodeList' pointer was utilized before it was verified against nullptr\. Check lines: 481, 482\. clientImpl\.c 481
1. V595 The 'pReq' pointer was utilized before it was verified against nullptr\. Check lines: 2831, 2832\. clientImpl\.c 2831
1. V595 The 'vgroup\_ids' pointer was utilized before it was verified against nullptr\. Check lines: 83, 84\. taos\_counter\.c 83
1. V595 The 'pReq' pointer was utilized before it was verified against nullptr\. Check lines: 1214, 1215\. transCli\.c 1214
1. V595 The 'pReq' pointer was utilized before it was verified against nullptr\. Check lines: 2755, 2758\. transCli\.c 2755
1. V595 The 'pReq' pointer was utilized before it was verified against nullptr\. Check lines: 3364, 3380\. transCli\.c 3364
1. **This is where I got bored** and stopped writing out warnings\. In fact, there are more errors than that\.


</details>
## Resource leaks

Manual memory management is fraught with errors\. The TDengine project is mostly written in C, so it's not surprising that we may encounter errors of this type\.

### Errors N131–N140\. Memory leak on a program execution flow

```cpp
taos_map_t *taos_map_new() {
  int r = 0;

  taos_map_t *self = (taos_map_t *)taos_malloc(sizeof(taos_map_t));
  self->size = 0;
  self->max_size = TAOS_MAP_INITIAL_SIZE;

  self->keys = taos_linked_list_new();
  if (self->keys == NULL) return NULL;
  ....
}
```

The PVS\-Studio warning: [V773](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'self' pointer\. A memory leak is possible\. taos\_map\.c 82

If a new key can't be created, the `taos_map_new` function terminates prematurely\. This does not free the memory buffer pointed to by `self`\. 

<details>
   <summary>Check out similar errors\\\.</summary>

1. V773 The function was exited without releasing the 'new\_addrs' pointer\. A memory leak is possible\. taos\_map\.c 289
1. V773 The function was exited without releasing the 'k' pointer\. A memory leak is possible\. taos\_metric\.c 52
1. V773 The function was exited without releasing the 'new\_nexts' pointer\. A memory leak is possible\. regex\_internal\.c 1421
1. V773 The function was exited without releasing the 'new\_indices' pointer\. A memory leak is possible\. regex\_internal\.c 1421
1. V773 The function was exited without releasing the 'new\_edests' pointer\. A memory leak is possible\. regex\_internal\.c 1421
1. V773 The function was exited without releasing the 'new\_eclosures' pointer\. A memory leak is possible\. regex\_internal\.c 1421
1. V773 The function was exited without releasing the 'self' pointer\. A memory leak is possible\. taos\_collector\_registry\.c 53
1. V773 The function was exited without releasing the 'new\_start' pointer\. A memory leak is possible\. regexec\.c 534
1. V773 The function was exited without releasing the 'new\_end' pointer\. A memory leak is possible\. regexec\.c 534


</details>
### Errors N141–N144\. Careless use of the realloc function

```cpp
INLINE void addDIA_Data(DynamicIntArray *dia, int value)
{
  if(dia->size==dia->capacity)
  {
    dia->capacity = dia->capacity << 1;
    dia->array = (unsigned char *)
      realloc(dia->array, dia->capacity*sizeof(unsigned char));
  }
  dia->array[dia->size] = (unsigned char)value;
  dia->size ++;
}
```

The PVS\-Studio warning: [V701](https://pvs-studio.com/en/docs/warnings/v701/) realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'dia\-\>array' is lost\. Consider assigning realloc\(\) to a temporary pointer\. DynamicIntArray\.c 54

If the [`realloc`](https://en.cppreference.com/w/c/memory/realloc) function fails to allocate a new buffer, it will return `NULL`\. The old value of the `dia->array` pointer will be lost and it will be impossible to free the buffer whose address was previously stored in it\.

Given the lack of a pointer check further, the memory leak is minor\. However, I'd like to highlight this improper handling of the `realloc` function\.

**Note**\. In this fragment, the memory allocation error will lead to interesting consequences\. Data will be written\.\.\.

```cpp
dia->array[dia->size] = (unsigned char)value;
```

\.\.\.not to a null pointer, but to some remote memory area, the address of which depends on the previous array size\. Consequences are unpredictable\. [Access violation](https://pvs-studio.com/en/blog/terms/0063/) is possible\. Perhaps, some data in memory will be corrupted but the program will continue to work, at least for a while\. As they say, happy debugging :\)

Other similar errors:

1. V701 realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'dba\-\>array' is lost\. Consider assigning realloc\(\) to a temporary pointer\. DynamicByteArray\.c 57
1. V701 realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'dba\-\>array' is lost\. Consider assigning realloc\(\) to a temporary pointer\. DynamicByteArray\.c 68
1. V701 realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'self\-\>str' is lost\. Consider assigning realloc\(\) to a temporary pointer\. taos\_string\_builder\.c 84

### Errors N145–N157\. Careless use of the emplace\_back function

```cpp
Status SstFileWriter::Open(const std::string& file_path) {
  ....
  for (size_t i = 0; i < user_collector_factories.size(); i++) {
    int_tbl_prop_collector_factories.emplace_back(
        new UserKeyTablePropertiesCollectorFactory(
            user_collector_factories[i]));
  }
  ....
}
```

The PVS\-Studio warning: [V1023](https://pvs-studio.com/en/docs/warnings/v1023/) A pointer without owner is added to the 'int\_tbl\_prop\_collector\_factories' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. sst\_file\_writer\.cc 298

If the container is full, memory is reallocated\. This operation may fail, resulting in a `std::bad_alloc` exception\. In this case, the pointer will be lost and the created object will never be deleted\.

A secure construction that protects against potential memory leaks:

```cpp
int_tbl_prop_collector_factories.emplace_back(
  std::make_unique<UserKeyTablePropertiesCollectorFactory>(
    user_collector_factories[i]));
```

<details>
   <summary>Other warnings\\\.</summary>

1. V1023 A pointer without owner is added to the 'locations' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. ConnectedElementLocationFilter\.cpp 54
1. V1023 A pointer without owner is added to the 'locations' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. ConnectedElementLocationFilter\.cpp 67
1. V1023 A pointer without owner is added to the 'outOERs' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. MaximalEdgeRing\.cpp 117
1. V1023 A pointer without owner is added to the 'edgeRings' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. PolygonBuilder\.cpp 87
1. V1023 A pointer without owner is added to the 'copied\_operands\_' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. merge\_context\.h 41
1. V1023 A pointer without owner is added to the 'copied\_operands\_' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. merge\_context\.h 57
1. V1023 A pointer without owner is added to the 'jobs' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. db\_impl\_compaction\_flush\.cc 458
1. V1023 A pointer without owner is added to the 'parent\_iters\_' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. range\_del\_aggregator\.cc 373
1. V1023 A pointer without owner is added to the 'builder\_guards' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. version\_set\.cc 5026
1. V1023 A pointer without owner is added to the 'table\_properties\_collectors' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. block\_based\_table\_builder\.cc 533
1. V1023 A pointer without owner is added to the 'table\_properties\_collectors' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. block\_based\_table\_builder\.cc 540
1. V1023 A pointer without owner is added to the 'int\_tbl\_prop\_collector\_factories' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. sst\_file\_writer\.cc 290


</details>
## Buffer/array overflow

The high performance of the C and C\+\+ languages comes at the cost of many automatic checks being absent\. There are also no built\-in checks for allocated buffer overflows\. Compilers partially compensate for the lack of checks by performing static analysis and issuing warnings for obvious cases\. However, it is also useful to use specialized tools such as PVS\-Studio that can detect even more errors\.

### Error N158\. Buffer overflow

```cpp
const char* rocksdb_iter_value(const rocksdb_iterator_t* iter, size_t* vlen) {
  Slice s = iter->rep->value();
  *vlen = s.size();
  return s.data();
}

int32_t streamDefaultIterGet_rocksdb(....) {
  ....
  while (rocksdb_iter_valid(pIter)) {
    const char* key = rocksdb_iter_key(pIter, &klen);
    int32_t     vlen = 0;
    const char* vval = rocksdb_iter_value(pIter, (size_t*)&vlen);
  ....
}
```

The PVS\-Studio warning: [V512](https://pvs-studio.com/en/docs/warnings/v512/) A call of the 'rocksdb\_iter\_value' function will lead to overflow of the buffer '& vlen'\. streamBackendRocksdb\.c 4390

This code will work on the 32\-bit app version but fail on a 64\-bit build\.

The address of the `vlen` 32\-bit variable is interpreted as a pointer to the type `size_t`:

```cpp
int32_t     vlen = 0;
const char* vval = rocksdb_iter_value(pIter, (size_t*)&vlen);
```

In the `rocksdb_iter_value` function, a value of type `size_t` will be written at this address\.

In a 32\-bit program—if we do not consider exotic architectures—the size of the `size_t` variable is 4 bytes and coincides with the size of the `int32_t` type\. The code will work correctly\.

In a 64\-bit program, the size of `size_t` type equals 8 bytes\. So, writing a 64\-bit value by the address of the `vlen` variable causes some data to be written outside this variable\. As a result, 4 bytes will be written to the stack after this variable, resulting in undefined program behavior\.

### Errors N159\. Buffer overflow due to confusion in constants

First, let's keep in mind that `MAX_QUERY_VALUE_LEN` is 1024:

```cpp
#define MAX_QUERY_VALUE_LEN       1024
```

Next, note that the third dimension of the array `char data[100][100][100][1024]` is also 1024:

```cpp
typedef struct _script_t {
  ....
  char              cols[12];
  char              data[100][100][1024];
  char              system_exit_code[12];
  ....
} SScript;
```

Finally, here is the code containing an error:

```cpp
bool simExecuteNativeSqlCommand(SScript *script, char *rest, bool isSlow) {
  ....
  char *value = NULL;
  if (i < MAX_QUERY_COL_NUM) {
    value = script->data[numOfRows][i];
  }
  if (value == NULL) {
    continue;
  }
  ....
  int32_t    *length = taos_fetch_lengths(pSql);
  ....
  if (length[i] < 0 || length[i] > 1 << 20) {
    fprintf(stderr, "Invalid length(%d) of BINARY or NCHAR\n", length[i]);
    exit(-1);
  }
  memset(value, 0, MAX_QUERY_VALUE_LEN);
  memcpy(value, row[i], length[i]);
  value[length[i]] = 0;  
  ....
}
```

The PVS\-Studio warning: [V512](https://pvs-studio.com/en/docs/warnings/v512/) A call of the 'memcpy' function will lead to overflow of the buffer 'value'\. simExec\.c 786

There is some value in an element of the `length[i]` array\. It's pre\-checked:

```cpp
if (length[i] < 0 || length[i] > 1 << 20) {
  fprintf(stderr, "Invalid length(%d) of BINARY or NCHAR\n", length[i]);
  exit(-1);
}
```

This value is then used as the size of the data to be copied:

```cpp
memcpy(value, row[i], length[i]);
```

The problem is that `1 << 20` is not 1024, but 1048576\. So, the check doesn't save from a potential buffer overflow\. I think the correct thing to do is to use a named constant `MAX_QUERY_VALUE_LEN` in the condition, rather than a magic number\. This constant should also be used when declaring the array\.

```cpp
char        data[100][100][MAX_QUERY_VALUE_LEN];  
....
if (length[i] < 0 || length[i] > MAX_QUERY_VALUE_LEN) {
  fprintf(stderr, "Invalid length(%d) of BINARY or NCHAR\n", length[i]);
  exit(-1);
}
memset(value, 0, MAX_QUERY_VALUE_LEN);
memcpy(value, row[i], length[i]);
```

Oops, I just realized there's another error in the code:

```cpp
value[length[i]] = 0;
```

This line is unnecessary and even harmful\. First, the array is already pre\-filled with zeros after the `memset` function call and the additional null terminator is not needed\. Second, if `length[i] == MAX_QUERY_VALUE_LEN`, the array overflows\. Basically, the author should delete this line\.

Let's dwell on this error a bit more\. Since we assume the null terminator, we cannot copy `MAX_QUERY_VALUE_LEN` bytes\. Then there will be no room for the null terminator\. Hence, we need to modify the check once again and use the `>=` operator instead of `>`\.

```cpp
if (length[i] < 0 || length[i] >= MAX_QUERY_VALUE_LEN)
```

### Error N160\. Potential array overflow

Let's take a moment to recap\. Can you imagine we've already gotten to 160 bugs?\!

![1249_TDengine_4/image2.png](https://import.viva64.com/docx/blog/1249_TDengine_4/image2.png)

And do you realize that we've already found 160 errors in the database? That's so unfortunate\.

I prescribe the immediate introduction of static code analyzers into the TDengine development process :\)

```cpp
dictionary * iniparser_load(const char * ininame)
{
  ....
  char line    [ASCIILINESZ+1] ;
  ....
  memset(line,    0, ASCIILINESZ);
  ....
  last=0 ;

  while (fgets(line+last, ASCIILINESZ-last, in)!=NULL) {
    lineno++ ;
    len = (int)strlen(line)-1;
    if (len==0)
      continue;
    /* Safety check against buffer overflows */
    if (line[len]!='\n') {
      fprintf(stderr,
              "iniparser: input line too long in %s (%d)\n",
              ininame,
              lineno);
      dictionary_del(dict);
      fclose(in);
      return NULL ;
    }
    ....
}
```

The PVS\-Studio warning: [V557](https://pvs-studio.com/en/docs/warnings/v557/) Array underrun is possible\. The value of 'len' index could reach \-1\. iniparser\.c 695

If the input for `fgets` is an empty string, the `len` variable will be \-1, which leads to an array overflow\. We've gone through it in the article: "[Shoot yourself in the foot when handling input data](https://pvs-studio.com/en/blog/posts/cpp/0599/)"\. This note considers identical reproduced errors\.

Given the above, this comment in the code looks even more ironic:

```cpp
/* Safety check against buffer overflows */
```

## Typos

Various programming languages have different protection measures against null pointers/references, array overflows, and division by zero\. Some, like C and C\+\+, rely entirely on a developer\. Others throw exceptions\. However, no language is immune to typos\. It's hard to determine what a typo is, so there are no standard, language\-level methods for dealing with them\.

But that doesn't mean that nothing can be done\. PVS\-Studio analyzer detects a large number of common types of typos\. While it doesn't implement a universal approach, it handles a wide range of typical cases—and that makes it highly effective in practice\.

### Error N161\. Overwriting value

```cpp
static int32_t getRowsBlockWithinMergeLimit(....) {
  ....
  if (keepRows == 0) {
    *pSkipBlock = true;
    *pRes = pOrigBlk;
  }

  *pSkipBlock = false;
  ....
}
```

The PVS\-Studio warning: [V519](https://pvs-studio.com/en/docs/warnings/v519/) The '\* pSkipBlock' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 2198, 2202\. tsort\.c 2202

The line `*pSkipBlock = true` makes no sense, as the value will change to `false` anyway\. Most likely, the author should have added `else`:

```cpp
if (keepRows == 0) {
  *pSkipBlock = true;
  *pRes = pOrigBlk;
}
else {
  *pSkipBlock = false;
}
```

### Error N162\. Copy\-paste repeat

```cpp
static void processSimpleMeta(SMqMetaRsp* pMetaRsp, cJSON** meta) {
  ...
  } else if (pMetaRsp->resMsgType == TDMT_VND_ALTER_TABLE) {
    processAlterTable(pMetaRsp, meta);
  } else if (pMetaRsp->resMsgType == TDMT_VND_DROP_TABLE) {
    processDropTable(pMetaRsp, meta);
  } else if (pMetaRsp->resMsgType == TDMT_VND_DROP_TABLE) {
    processDropTable(pMetaRsp, meta);
  } else if (pMetaRsp->resMsgType == TDMT_VND_DELETE) {
  ....
}
```

The PVS\-Studio warning: [V517](https://pvs-studio.com/en/docs/warnings/v517/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 2316, 2318\. clientRawBlockWrite\.c 2316

The above blocks of the same\-type test might have been written using the copy\-paste method\. At some point, the developer probably got distracted, and repeated this snippet:

```cpp
} else if (pMetaRsp->resMsgType == TDMT_VND_DROP_TABLE) {
  processDropTable(pMetaRsp, meta);
```

This is a fairly common type of error\. [We have plenty of them in our collection](https://pvs-studio.com/en/blog/examples/v517/)\. If one of the blocks is redundant, the author can safely remove it\. In this case, the typo doesn't affect the program operation\. In the worst case, the developer implied another check or another action here\.

### Error N163\. Forgotten dangerous code with possible division by 0 

```cpp
OffsetSegmentGenerator::OffsetSegmentGenerator(....) : ....
{
  ....
  // compute intersections in full precision, to provide accuracy
  // the points are rounded as they are inserted into the curve line
  filletAngleQuantum = MATH_PI / 2.0 / bufParams.getQuadrantSegments();

  int quadSegs = bufParams.getQuadrantSegments();
  if (quadSegs < 1) quadSegs = 1;
  filletAngleQuantum = MATH_PI / 2.0 / quadSegs;
  ....
}
```

The PVS\-Studio warning: [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'filletAngleQuantum' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 82, 86\. OffsetSegmentGenerator\.cpp 86

The author wrote the following code:

```cpp
filletAngleQuantum = MATH_PI / 2.0 / bufParams.getQuadrantSegments();
```

The `getQuadrantSegments` function can return 0, so the code was rewritten to protect against division by zero:

```cpp
int quadSegs = bufParams.getQuadrantSegments();
if (quadSegs < 1) quadSegs = 1;
filletAngleQuantum = MATH_PI / 2.0 / quadSegs;
```

Except, they forgot to delete the previous line\. As a result, we can end up with the division by zero and, as a result, undefined behavior\.

### Error N164\. Identical functions

```cpp
typedef struct {
  int64_t firstVer;
  int64_t lastVer;
  int64_t createTs;
  int64_t closeTs;
  int64_t fileSize;
  int64_t syncedOffset;
} SWalFileInfo;

static inline int64_t walGetCurFileFirstVer(SWal* pWal) {
  if (pWal->writeCur == -1) return -1;
  SWalFileInfo* pInfo =
    (SWalFileInfo*)taosArrayGet(pWal->fileInfoSet, pWal->writeCur);
  return pInfo->firstVer;
}

static inline int64_t walGetCurFileLastVer(SWal* pWal) {
  if (pWal->writeCur == -1) return -1;
  SWalFileInfo* pInfo =
    (SWalFileInfo*)taosArrayGet(pWal->fileInfoSet, pWal->writeCur);
  return pInfo->firstVer;
}
```

The PVS\-Studio warning: [V524](https://pvs-studio.com/en/docs/warnings/v524/) It is odd that the body of 'walGetCurFileLastVer' function is fully equivalent to the body of 'walGetCurFileFirstVer' function\. walInt\.h 97

There are two data members in the `SWalFileInfo` structure:

* **first**Ver;
* **last**Ver\.

There are two functions to obtain values from these data members:

* walGetCurFile**First**Ver;
* walGetCurFile**Last**Ver\.

But the function bodies are identical and both return the value of the `firstVer` data member\.

### Errors N165–N170\. "Parentheses curse"

```cpp
int32_t dmInit() {
  dInfo("start to init dnode env");
  int32_t code = 0;
  ....
  if ((code = dmCheckDiskSpace()) != 0) return code;
  if ((code = dmCheckRepeatInit(dmInstance())) != 0) return code;
  if ((code = dmInitSystem()) != 0) return code;
  if ((code = dmInitMonitor()) != 0) return code;
  if ((code = dmInitAudit()) != 0) return code;
  if ((code = dmInitDnode(dmInstance())) != 0) return code;
  if ((code = InitRegexCache() != 0)) return code;
  ....
}
```

The PVS\-Studio warning: [V593](https://pvs-studio.com/en/docs/warnings/v593/) Consider reviewing the expression of the 'A \= B \!\= C' kind\. The expression is calculated as following: 'A \= \(B \!\= C\)'\. dmEnv\.c 182

Who spotted the error? ;\)

A nice typo, I think\. The parenthesis is misplaced in the last check\. The result of the `InitRegexCache` function call is compared to 0, and only after 0 or 1, it is written to the `code` variable\.

Here are a few other similar typos:

1. V593 Consider reviewing the expression of the 'A \= B \!\= C' kind\. The expression is calculated as following: 'A \= \(B \!\= C\)'\. mndArbGroup\.c 299
1. V593 Consider reviewing the expression of the 'A \= B \!\= C' kind\. The expression is calculated as following: 'A \= \(B \!\= C\)'\. mndConfig\.c 430
1. V593 Consider reviewing the expression of the 'A \= B \!\= C' kind\. The expression is calculated as following: 'A \= \(B \!\= C\)'\. mndUser\.c 418
1. V593 Consider reviewing the expression of the 'A \= B < C' kind\. The expression is calculated as following: 'A \= \(B < C\)'\. streamMeta\.c 411
1. V593 Consider reviewing the expression of the 'A \= B \!\= C' kind\. The expression is calculated as following: 'A \= \(B \!\= C\)'\. transCli\.c 1876

### Error N171\. Pointless check

```cpp
void
CoordinateSequence::add(const CoordinateSequence& cs,
                        std::size_t from, std::size_t to)
{
  if (cs.stride() == stride() && cs.hasM() == cs.hasM()) {
      m_vect.insert(m_vect.end(),
                    std::next(cs.m_vect.cbegin(),
                    static_cast<std::ptrdiff_t>(from * stride())),
                    std::next(cs.m_vect.cbegin(),
                    static_cast<std::ptrdiff_t>((to + 1u)*stride())));
  } else {
  ....
}
```

The PVS\-Studio warning: [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\=\=' operator: cs\.hasM\(\) \=\= cs\.hasM\(\) CoordinateSequence\.cpp 154

The developer's hand trembled and accidentally added an extra `cs.`\. As a result, some part of the condition will always be true:

```cpp
cs.hasM() == cs.hasM()
```

### Error N172\. Not an error but still an error

```cpp
bool startsWith(const std::string & s, char prefix) {
  if (s.empty() == 0) {
    return false;
  }

  return s[0] == prefix;
}
```

The PVS\-Studio warning: [V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The '0' index is pointing beyond array bound\. string\.cpp 53

The analyzer's warning isn't quite correct here\. In fact, there is no array overflow\. Even an empty string \(the null\-terminated string\) contains at least one character\. However, the analyzer points out that if the string is empty, there is no point to access a null element\. It's weird and it's probably some kind of a typo\.

**Note N1**\. Perhaps, we should refine this diagnostic rule to show another message for such a case\. I'll open a task for my colleagues\.

**Note N2**\. Before C\+\+11, such code was considered erroneous as it [causes](https://en.cppreference.com/w/cpp/string/basic_string/operator_at) undefined behavior\.

The error is that the result of the `empty` function call is compared to 0\. The comparison is obviously unnecessary and the correct code should look like this:

```cpp
bool startsWith(const std::string & s, char prefix) {
  if (s.empty()) {
    return false;
  }

  return s[0] == prefix;
}
```

### Error N173\. Typo when using similar variable names

```cpp
int32_t streamTaskUpdateTaskCheckpointInfo(....) {
  ....
  bool valid = (pInfo->checkpointId  <= pReq->checkpointId &&
                pInfo->checkpointVer <= pReq->checkpointVer &&
                pInfo->processedVer  <= pReq->checkpointVer);
  ....
}
```

The PVS\-Studio warning: [V1013](https://pvs-studio.com/en/docs/warnings/v1013/) Suspicious subexpression in a sequence of similar comparisons\. streamCheckpoint\.c 654

The variable names look similar, so the typo is not surprising\.

The `pInfo->processedVer` variable should be compared to `pReq->processedVer` instead of comparing with `pReq->checkpointVer`\.

### Errors N174 and N175\. Sausage and the price for laziness

I've already broken down two other typos in previous articles on refactoring sloppy code:

1. Breaking down bugs in TDengine to master refactoring, part 1: [sausage code](https://pvs-studio.com/en/blog/posts/cpp/1230/)
1. Breaking down bugs in TDengine to master refactoring, part 3: [price of laziness](https://pvs-studio.com/en/blog/posts/cpp/1242/)

## Other errors

The project will benefit greatly from fixing the errors we've reviewed in this article\. However, this piece isn't meant to serve as a guide to the errors\. I've already mentioned that in the introduction, but let me say it again:

1. I skimmed through the report and noted only a portion of these issues\. My purpose was to show the benefits of static code analysis as a practice rather than find as many errors as possible\.
1. [Static analysis is most efficient when used regularly, not occasionally](https://habr.com/en/articles/440610/)\.

As a conclusion, let's look at a few more various errors not mentioned in the above sections\. 

### Errors N176 and N177\. Errors when using the shift operator <<

```cpp
uint64_t unpackUint64(uint8_t* ch, uint8_t sz) {
  uint64_t n = 0;
  for (uint8_t i = 0; i < sz; i++) {
    n = n | (ch[i] << (8 * i));
  }
  return n;
}
```

The PVS\-Studio warning: [V629](https://pvs-studio.com/en/docs/warnings/v629/) Consider inspecting the 'ch\[i\] << \(8 \* i\)' expression\. Bit shifting of the 32\-bit value with a subsequent expansion to the 64\-bit type\. indexFstUtil\.c 55

A 64\-bit value must be created from a byte array\. However, a failure will occur when attempting to set the upper 32\-bits in a 64\-bit variable\. On shift, the left operand—an 8\-bit unsigned character—will be implicitly converted to a 32\-bit `int`\. However, it's not enough: if the `int` value is shifted by more than 31 bits, an overflow will occur\. The developer must explicitly cast the operand to a 64\-bit type beforehand:

```cpp
n = n | ((uint64_t)(ch[i]) << (8 * i));
```

Here is a similar error:

```cpp
static int hashset_add(hashset_t set, void *item) {
  int ret = hashset_add_member(set, item);

  size_t old_capacity = set->capacity;
  if (set->nitems >= (double)old_capacity * set->load_factor) {
    size_t *old_items = set->items;
    ++set->nbits;
    set->capacity = (size_t)(1 << set->nbits);
  ....
}
```

But the PVS\-Studio warning is different: [V1028](https://pvs-studio.com/en/docs/warnings/v1028/) Possible overflow\. Consider casting operands of the '1 << set\-\>nbits' operator to the 'size\_t' type, not the result\. tdbPager\.c 88

### Error N178\. Unreachable code

How about a little attention test? Try to find the bug in this function:

```cpp
static int32_t getBlkFromSessionCache(struct SOperatorInfo* pOperator,
  int64_t sessionId, SGcSessionCtx* pSession, SSDataBlock** ppRes)
{
  int32_t code = TSDB_CODE_SUCCESS;
  SGroupCacheOperatorInfo* pGCache = pOperator->info;
  bool locked = false;
  SGcDownstreamCtx* pCtx = &pGCache->pDownstreams[pSession->downstreamIdx];
  
  while (true) {
    bool got = false;
    code = getBlkFromSessionCacheImpl(pOperator, sessionId,
                                      pSession, ppRes, &got);
    if (TSDB_CODE_SUCCESS != code || got) {
      goto _return;
    }
    
    if ((atomic_load_64(&pCtx->fetchSessionId) == sessionId)
      || (-1 == atomic_val_compare_exchange_64(
                  &pCtx->fetchSessionId, -1, sessionId))) {
      if (locked) {
        (void)taosThreadMutexUnlock(&pSession->pGroupData->mutex);
        locked = false;
      }
      
      code = getCacheBlkFromDownstreamOperator(pOperator, pCtx,
                                               sessionId, pSession, ppRes);
      goto _return;
    } else {
      // FOR NOW, SHOULD NOT REACH HERE
      qError("Invalid fetchSessionId:%" PRId64 ",
             currentSessionId:%" PRId64, pCtx->fetchSessionId, sessionId);
      return TSDB_CODE_QRY_EXECUTOR_INTERNAL_ERROR;
    }

    if (locked) {
      code = groupCacheSessionWait(pOperator, pCtx, sessionId,
                                   pSession, ppRes);
      locked = false;
      if (TSDB_CODE_SUCCESS != code) {
        goto _return;
      }
      
      break;
    }
    
    (void)taosThreadMutexLock(&pSession->pGroupData->mutex);
    locked = true;
  };


_return:

  if (locked) {
    (void)taosThreadMutexUnlock(&pSession->pGroupData->mutex);
  }

  return code;
}
```

The PVS\-Studio warning: [V779](https://pvs-studio.com/en/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. groupcacheoperator\.c 1227

Any luck? If not, the error is hiding here:

```cpp
if (....)        // << (A)
{
  ....
  goto _return;  // << (B)
} else {
  ....
  return TSDB_CODE_QRY_EXECUTOR_INTERNAL_ERROR; // << (C)
}
....
if (locked) {    // << (D)
....
_return:         // << (E)
```

Regardless of the condition \(A\), the code \(D\) will never get control\. We will either move \(B\) to the label \(E\) or exit the function \(C\)\.

### Errors N179\-N184\. Potential overflow

```cpp
typedef struct SFilePage {
  int32_t num;
  ....
} SFilePage;

typedef struct tMemBucket {
  ....
  int32_t            bytes;
  ....
} tMemBucket;

static int32_t loadDataFromFilePage(tMemBucket *pMemBucket, ....) {
  ....
  SFilePage *pg = getBufPage(pMemBucket->pBuffer, *pageId);
  ....
  (void)memcpy((*buffer)->data + offset, pg->data,
               (size_t)(pg->num * pMemBucket->bytes));
  ....
}
```

The PVS\-Studio warning: [V1028](https://pvs-studio.com/en/docs/warnings/v1028/) Possible overflow\. Consider casting operands of the 'pg\-\>num \* pMemBucket\-\>bytes' operator to the 'size\_t' type, not the result\. tpercentile\.c 64

Explicit type casting doesn't help avoid an overflow when multiplying 32\-bit variables\.

```cpp
(size_t)(pg->num * pMemBucket->bytes)
```

The author should perform type casting before multiplication, not after:

```cpp
(size_t)(pg->num) * pMemBucket->bytes
```

Look at similar warnings:

1. V1028 Possible overflow\. Consider casting operands of the 'pColData\-\>nVal \+ 1' operator to the 'int64\_t' type, not the result\. tdataformat\.c 1904
1. V1028 Possible overflow\. Consider casting operands of the 'pColData\-\>nVal \+ 1' operator to the 'int64\_t' type, not the result\. tdataformat\.c 1904
1. V1028 Possible overflow\. Consider casting operands, not the result\. compaction\_picker\_level\.cc 818
1. V1028 Possible overflow\. Consider casting operands of the 'vlen \* 4' operator to the 'size\_t' type, not the result\. tbase64\.c 23
1. V1028 Possible overflow\. Consider casting operands of the 'inlen \* 3' operator to the 'size\_t' type, not the result\. tbase64\.c 59

### Error N185\. Incorrect std namespace extension

```cpp
namespace std {
inline void swap(ROCKSDB_NAMESPACE::port::WindowsThread& th1,
                 ROCKSDB_NAMESPACE::port::WindowsThread& th2) {
  th1.swap(th2);
}
}  // namespace std
```

The PVS\-Studio warning: [V1061](https://pvs-studio.com/en/docs/warnings/v1061/) Extending the 'std' namespace may result in undefined behavior\. win\_thread\.h 110

You may learn more about this defect in the documentation if you'd like to\. Honestly, I'm getting tired of going over all these bugs :\) I'm going to hit the 190 mark and then I'm done\. Or shall I reach 200 anyway? No, down with perfectionism\! I need a break\.

### Error N186\. Comparing of "garbage" bytes

```cpp
typedef struct STreeNode {
  int32_t index;
  void   *pData;  // TODO remove it?
} STreeNode;

int32_t tMergeTreeAdjust(SMultiwayMergeTreeInfo* pTree, int32_t idx) {
  ....
  STreeNode kLeaf = pTree->pNode[idx];
  ....
  if (memcmp(&kLeaf, &pTree->pNode[1], sizeof(kLeaf)) != 0) {
  ....
}
```

The PVS\-Studio warning: [V1103](https://pvs-studio.com/en/docs/warnings/v1103/) The values of padding bytes are unspecified\. Comparing objects with padding using 'memcmp' may lead to unexpected result\. tlosertree\.c 127

In a 64\-bit program, there are 4 additional bytes between the `index` variable and the pointer intended to align the 64\-bit pointer to the 8\-byte boundary\. It's a bad idea to compare such structures using the `memcmp` function\. The additional bytes for alignment contain random values\. The `memcmp` function may consider the structures as different even though the values of all fields are the same\.

### Errors N187\-N190\. Use of obsolete functions related to cryptography

```cpp
uint32_t taosSafeRand(void) {
  ....
  if (!CryptGenRandom(hCryptProv, 4, &seed)) return seed;
  ....
}
```

The PVS\-Studio warning: [V1109](https://pvs-studio.com/en/docs/warnings/v1109/) The 'CryptGenRandom' function is deprecated\. Consider switching to an equivalent newer function\. osRand\.c 56

Other similar warnings:

1. V1109 The 'CryptAcquireContextA' function is deprecated\. Consider switching to an equivalent newer function\. osRand\.c 50
1. V1109 The 'CryptAcquireContextA' function is deprecated\. Consider switching to an equivalent newer function\. osRand\.c 51
1. V1109 The 'CryptReleaseContext' function is deprecated\. Consider switching to an equivalent newer function\. osRand\.c 58

## Conclusion

Static analysis enables users to meet various technical needs listed below\. 

1. Enhance software quality and reliability while minimizing reputational risks associated with zero\-day vulnerabilities\.
1. Detect bugs and potential vulnerabilities at the development stage: the earlier a defect is detected, the cheaper it is to fix\.
1. Identify error patterns that your team may not even be aware of\.
1. Shift the focus during code reviews toward algorithms and high\-level bugs rather than scrutinize variable names and parentheses placement\.
1. Write simpler and more reliable code\. Even the analyzer's false positives may be useful\. If the code confuses an analyzer, it's likely to confuse a human too\. It's better to rewrite this code\. 
1. Maintain overall code quality\. For example, an increase in error density might indicate the need for better training for new team members\.
1. Build a secure software development lifecycle \(SSDLC\)\.

[PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) can be used for all these tasks\. It supports code analysis for C, C\+\+, C\#, and Java\. It runs on Windows, Linux, and macOS\. PVS\-Studio is a [SAST solution](https://pvs-studio.com/en/pvs-studio/sast/) to enhance quality, reliability, and security of your projects\.

## Additional links

1. [C\+\+ tools evolution: static code analyzers](https://pvs-studio.com/en/blog/posts/cpp/0873/)\.
1. [Static analyzer nudges you to write clean code](https://pvs-studio.com/en/blog/posts/cpp/1115/)\.
1. [5 reasons why static analysis is important for business](https://pvs-studio.com/en/blog/posts/1046/)\.
1. [The code analyzer is wrong\. Long live the analyzer\!](https://pvs-studio.com/en/blog/posts/cpp/0779/)\.
1. [How to introduce a static code analyzer in a legacy project and not to discourage the team](https://pvs-studio.com/en/blog/posts/0743/)\.