# How to shoot yourself in the foot in C and C\+\+\. Haiku OS Cookbook

The story of how the PVS\-Studio static analyzer and the Haiku OS code met goes back to the year 2015\.  It was an exciting experiment and useful experience for teams of both projects\. Why the experiment? At that moment, we didn't have the analyzer for Linux and we wouldn't have it for another year and a half\. Anyway, efforts of enthusiasts from our team have been rewarded: we got acquainted with Haiku developers and increased the code quality, widened our error base with rare bugs made by developers and refined the analyzer\. Now you can check the Haiku code for errors easily and quickly\.

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

## Introduction

Meet the main characters of our story \- the [Haiku](https://www.haiku-os.org/) with open source code and the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) static analyzer for C, C\+\+, C\# and Java\. When we had a dig into the project analysis 4\.5 years ago, we had to deal only with the compiled executable analyzer file\.  All infrastructure for parsing compiler parameters, running a preprocessor, paralleling the analysis and so on was taken from the utility [Compiler Monitoring UI](https://pvs-studio.com/en/docs/manual/0031/), written in C\#\. That utility was ported in parts to the Mono platform to be run in Linux\. The Haiku project is built using the cross compiler under various OSs, except Windows\. Once again, I'd like to mention the convenience and documentation completeness related to Haiku building\. Also I'd like to thank Haiku developers for their help in building the project\.

It's much simpler to perform the analysis now\. Here is the list of all commands for building and analyzing the project:

```cpp
cd /opt
git clone https://review.haiku-os.org/buildtools
git clone https://review.haiku-os.org/haiku
cd ./haiku
mkdir generated.x86_64; cd generated.x86_64
../configure --distro-compatibility official -j12 \
  --build-cross-tools x86_64 ../../buildtools
cd ../../buildtools/jam
make all
cd /opt/haiku/generated.x86_64
pvs-studio-analyzer trace -- /opt/buildtools/jam/bin.linuxx86/jam \
  -q -j1 @nightly-anyboot
pvs-studio-analyzer analyze -l /mnt/svn/PVS-Studio.lic -r /opt/haiku \
   -C x86_64-unknown-haiku-gcc -o /opt/haiku/haiku.log -j12
```

By the way, the project analysis was implemented in a Docker container\. Recently we've prepared new documentation on this topic: [Running PVS\-Studio in Docker](https://pvs-studio.com/en/docs/manual/0047/)\. This can make it very easy for some companies to apply static analysis techniques for their projects\.

## Uninitialized variables

[V614](https://pvs-studio.com/en/docs/warnings/v614/) Uninitialized variable 'rval' used\. fetch\.c 1727

```cpp
int
auto_fetch(int argc, char *argv[])
{
  volatile int  argpos;
  int    rval;                  // <=
  argpos = 0;

  if (sigsetjmp(toplevel, 1)) {
    if (connected)
      disconnect(0, NULL);
    if (rval > 0)               // <=
      rval = argpos + 1;
    return (rval);
  }
  ....
}
```

The _rval _variable hasn't been initialized on declaration, so it's comparison with the null value will lead to undefined result\. If the circumstances fail, the uncertain value of the _rval _variable_ _can become a return value of the _auto\_fetch_ function\.

[V614](https://pvs-studio.com/en/docs/warnings/v614/) Uninitialized pointer 'res' used\. commands\.c 2873

```cpp
struct addrinfo {
 int ai_flags;
 int ai_family;
 int ai_socktype;
 int ai_protocol;
 socklen_t ai_addrlen;
 char *ai_canonname;
 struct sockaddr *ai_addr;
 struct addrinfo *ai_next;
};

static int
sourceroute(struct addrinfo *ai, char *arg, char **cpp,
            int *lenp, int *protop, int *optp)
{
  static char buf[1024 + ALIGNBYTES];
  char *cp, *cp2, *lsrp, *ep;
  struct sockaddr_in *_sin;
#ifdef INET6
  struct sockaddr_in6 *sin6;
  struct ip6_rthdr *rth;
#endif
  struct addrinfo hints, *res;     // <=
  int error;
  char c;

  if (cpp == NULL || lenp == NULL)
    return -1;
  if (*cpp != NULL) {
    switch (res->ai_family) {      // <=
    case AF_INET:
      if (*lenp < 7)
        return -1;
      break;
      ....
    }
  }
  ....
}
```

Here is a similar case of using the uninitialized variable, except that _res_ is an uninitialized pointer that takes place here\. 

[V506](https://pvs-studio.com/en/docs/warnings/v506/) Pointer to local variable 'normalized' is stored outside the scope of this variable\. Such a pointer will become invalid\. TextView\.cpp 5596

```cpp
void
BTextView::_ApplyStyleRange(...., const BFont* font, ....)
{
  if (font != NULL) {
    BFont normalized = *font;
    _NormalizeFont(&normalized);
    font = &normalized;
  }
  ....
  fStyles->SetStyleRange(fromOffset, toOffset, fText->Length(), mode,
    font, color);
}
```

The programmer probably needed to normalize the object using an intermediate variable\. But now the _font _pointer contains the pointer to the _normalized_ object, which will be removed after exiting the scope, where the temporary object was created\.

[V603](https://pvs-studio.com/en/docs/warnings/v603/) The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 27

```cpp
int8
BUnicodeChar::Type(uint32 c)
{
  BUnicodeChar();
  return u_charType(c);
}
```

A very common mistake among C\+\+ programmers is to use the constructor's call supposedly to initialize/nullify class fields\. In this case, modification of class fields doesn't happen, but a new unnamed object of this class is created and then immediately destroyed\. Unfortunately, there are plenty of such places in the project:

* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 37
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 49
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 58
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 67
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 77
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 89
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 103
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 115
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 126
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 142
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 152
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 163
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 186
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 196
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 206
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 214
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 222
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 230

[V670](https://pvs-studio.com/en/docs/warnings/v670/) The uninitialized class member 'fPatternHandler' is used to initialize the 'fInternal' member\. Remember that members are initialized in the order of their declarations inside a class\. Painter\.cpp 184

```cpp
Painter::Painter()
  :
  fInternal(fPatternHandler),
  ....
  fPatternHandler(),
  ....
{
  ....
};

class Painter {
  ....
private:
  mutable PainterAggInterface fInternal; // line 336

  bool fSubpixelPrecise : 1;
  bool fValidClipping : 1;
  bool fDrawingText : 1;
  bool fAttached : 1;
  bool fIdentityTransform : 1;

  Transformable fTransform;
  float fPenSize;
  const BRegion* fClippingRegion;
  drawing_mode fDrawingMode;
  source_alpha fAlphaSrcMode;
  alpha_function fAlphaFncMode;
  cap_mode fLineCapMode;
  join_mode fLineJoinMode;
  float fMiterLimit;

  PatternHandler fPatternHandler;        // line 355
  mutable AGGTextRenderer fTextRenderer;
};
```

Another example of incorrect initialization\. Class fields are initialized in the order of their declaration in the class itself\. In this example, the _fInternal_ field will be the first to initialize using the uninitialized _fPatternHandler_ value\.

## Suspicious \#define

[V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. subr\_gtaskqueue\.c 191

```cpp
#define  TQ_LOCK(tq)              \
  do {                \
    if ((tq)->tq_spin)          \
      mtx_lock_spin(&(tq)->tq_mutex);      \
    else              \
      mtx_lock(&(tq)->tq_mutex);      \
  } while (0)
#define  TQ_ASSERT_LOCKED(tq)  mtx_assert(&(tq)->tq_mutex, MA_OWNED)

#define  TQ_UNLOCK(tq)              \
  do {                \
    if ((tq)->tq_spin)          \
      mtx_unlock_spin(&(tq)->tq_mutex);    \
    else              \
      mtx_unlock(&(tq)->tq_mutex);      \
  } while (0)

void
grouptask_block(struct grouptask *grouptask)
{
  ....
  TQ_LOCK(queue);
  gtask->ta_flags |= TASK_NOENQUEUE;
  gtaskqueue_drain_locked(queue, gtask);
  TQ_UNLOCK(queue);
}
```

This code snippet doesn't look suspicious until you look at the preprocessor result:

```cpp
void
grouptask_block(struct grouptask *grouptask)
{
  ....
  do { if ((queue)->tq_spin) mtx_lock(&(queue)->tq_mutex);
       else mtx_lock(&(queue)->tq_mutex); } while (0);
  gtask->ta_flags |= 0x4;
  gtaskqueue_drain_locked(queue, gtask);
   do { if ((queue)->tq_spin) mtx_unlock(&(queue)->tq_mutex);
        else mtx_unlock(&(queue)->tq_mutex); } while (0);
}
```

The analyzer is truly right \- _if_ and _else_ branches are identical\. But where are the _mtx\_lock\_spin_ and _mtx\_unlock\_spin_ functions? Macros _TQ\_LOCK_, _TQ\_UNLOCK_ and the _grouptask\_block_ function are declared in one file almost next to each other, but nevertheless a replacement took place somewhere here\.

Searching through the files resulted only in _mutex\.h_ with the following content: 

```cpp
/* on FreeBSD these are different functions */
#define mtx_lock_spin(x)   mtx_lock(x)
#define mtx_unlock_spin(x) mtx_unlock(x)
```

Project developers should check it out whether such a replacement is correct or not\. I checked this project in Linux and such replacing seemed suspicious to me\.

## Errors with the free function

[V575](https://pvs-studio.com/en/docs/warnings/v575/) The null pointer is passed into 'free' function\. Inspect the first argument\. setmime\.cpp 727

```cpp
void
MimeType::_PurgeProperties()
{
  fShort.Truncate(0);
  fLong.Truncate(0);
  fPrefApp.Truncate(0);
  fPrefAppSig.Truncate(0);
  fSniffRule.Truncate(0);

  delete fSmallIcon;
  fSmallIcon = NULL;

  delete fBigIcon;
  fBigIcon = NULL;

  fVectorIcon = NULL;            // <=
  free(fVectorIcon);             // <=

  fExtensions.clear();
  fAttributes.clear();
}
```

You can pass the null pointer in the _free_ function, but such usage is definitely suspicious\. Thus, the analyzer found mixed up lines of code\. First, the code author had to release the memory by the _fVectorIcon _pointer, only after that assign _NULL_\.

[V575](https://pvs-studio.com/en/docs/warnings/v575/) The null pointer is passed into 'free' function\. Inspect the first argument\. driver\_settings\.cpp 461

```cpp
static settings_handle *
load_driver_settings_from_file(int file, const char *driverName)
{
  ....
  handle = new_settings(text, driverName);
  if (handle != NULL) {
    // everything went fine!
    return handle;
  }

  free(handle);           // <=
  ....
}
```

This is another example of explicit passing a null pointer to the _free_ function\. This line can be deleted, as the function exits after obtaining the pointer successfully\. 

[V575](https://pvs-studio.com/en/docs/warnings/v575/) The null pointer is passed into 'free' function\. Inspect the first argument\. PackageFileHeapWriter\.cpp 166

```cpp
void* _GetBuffer()
{
  ....
  void* buffer = malloc(fBufferSize);
  if (buffer == NULL && !fBuffers.AddItem(buffer)) {
    free(buffer);
    throw std::bad_alloc();
  }
  return buffer;
}
```

Someone made an error here\. The \|\|operator has to be used instead of &&\. Only in this case the _std::bad\_alloc\(\) _exception will be thrown in case if memory allocation \(using the _malloc_ function\) failed\. 

## Errors with the delete operator

[V611](https://pvs-studio.com/en/docs/warnings/v611/) The memory was allocated using 'new T\[\]' operator but was released using the 'delete' operator\. Consider inspecting this code\. It's probably better to use 'delete \[\] fMsg;'\. Err\.cpp 65

```cpp
class Err {
public:
 ....
private:
 char *fMsg;
 ssize_t fPos;
};

void
Err::Unset() {
 delete fMsg;                                   // <=
 fMsg = __null;
 fPos = -1;
}

void
Err::SetMsg(const char *msg) {
 if (fMsg) {
  delete fMsg;                                  // <=
  fMsg = __null;
 }
 if (msg) {
  fMsg = new(std::nothrow) char[strlen(msg)+1]; // <=
  if (fMsg)
   strcpy(fMsg, msg);
 }
}
```

The _fMsg _pointer is used to allocate memory for an array of characters\. The _delete _operator is used to release the memory instead of _delete\[\]_\.

[V611](https://pvs-studio.com/en/docs/warnings/v611/) The memory was allocated using 'new' operator but was released using the 'free' function\. Consider inspecting operation logics behind the 'wrapperPool' variable\. vm\_page\.cpp 3080

```cpp
status_t
vm_page_write_modified_page_range(....)
{
  ....
  PageWriteWrapper* wrapperPool
    = new(malloc_flags(allocationFlags)) PageWriteWrapper[maxPages + 1];
  PageWriteWrapper** wrappers
    = new(malloc_flags(allocationFlags)) PageWriteWrapper*[maxPages];
  if (wrapperPool == NULL || wrappers == NULL) {
    free(wrapperPool);                              // <=
    free(wrappers);                                 // <=
    wrapperPool = stackWrappersPool;
    wrappers = stackWrappers;
    maxPages = 1;
  }
  ....
}
```

Here _malloc\_flags_ is a function that calls_ malloc_\. Then _placement\-new_ constructs the object here\. As the _PageWriteWrapper_ class is implemented in the following way:

```cpp
class PageWriteWrapper {
public:
 PageWriteWrapper();
 ~PageWriteWrapper();
 void SetTo(vm_page* page);
 bool Done(status_t result);

private:
 vm_page* fPage;
 struct VMCache* fCache;
 bool fIsActive;
};

PageWriteWrapper::PageWriteWrapper()
 :
 fIsActive(false)
{
}

PageWriteWrapper::~PageWriteWrapper()
{
 if (fIsActive)
  panic("page write wrapper going out of scope but isn't completed");
}
```

the objects destructors of this class_ _won't be called due to the use of the _free_ function to release memory\.

[V611](https://pvs-studio.com/en/docs/warnings/v611/) The memory was allocated using 'new T\[\]' operator but was released using the 'delete' operator\. Consider inspecting this code\. It's probably better to use 'delete \[\] fOutBuffer;'\. Check lines: 26, 45\. PCL6Rasterizer\.h 26

```cpp
class PCL6Rasterizer : public Rasterizer
{
public:
  ....
  ~PCL6Rasterizer()
  {
    delete fOutBuffer;
    fOutBuffer = NULL;
  }
  ....
  virtual void InitializeBuffer()
  {
    fOutBuffer = new uchar[fOutBufferSize];
  }
private:
  uchar* fOutBuffer;
  int    fOutBufferSize;
};
```

It's a common error to use the _delete_ operator instead of _delete\[\]\._ It is easiest to make a mistake when writing a class, as the destructor's code is often far from the memory locations\. Here, the programmer incorrectly frees the memory stored by the _fOutBuffer_ pointer in the destructor\.

[V772](https://pvs-studio.com/en/docs/warnings/v772/) Calling a 'delete' operator for a void pointer will cause undefined behavior\. Hashtable\.cpp 207

```cpp
void
Hashtable::MakeEmpty(int8 keyMode,int8 valueMode)
{
  ....
  for (entry = fTable[index]; entry; entry = next) {
    switch (keyMode) {
      case HASH_EMPTY_DELETE:
        // TODO: destructors are not called!
        delete (void*)entry->key;
        break;
      case HASH_EMPTY_FREE:
        free((void*)entry->key);
        break;
    }
    switch (valueMode) {
      case HASH_EMPTY_DELETE:
        // TODO: destructors are not called!
        delete entry->value;
        break;
      case HASH_EMPTY_FREE:
        free(entry->value);
        break;
    }
    next = entry->next;
    delete entry;
  }
  ....
}
```

In addition to an incorrect choice between _delete_/_delete\[\] _and _free_, you can also run into undefined behavior when trying to clear the memory by a pointer to the void type _\(void\*\)_\.

## Functions without return value

[V591](https://pvs-studio.com/en/docs/warnings/v591/) Non\-void function should return a value\. Referenceable\.h 228

```cpp
BReference& operator=(const BReference<const Type>& other)
{
  fReference = other.fReference;
}
```

An overloaded assignment operator lacks a return value\. In this case, the operator will return a random value, which can lead to strange errors\.

Here are similar issues in other code fragments of this class:

* V591 Non\-void function should return a value\. Referenceable\.h 233
* V591 Non\-void function should return a value\. Referenceable\.h 239

[V591](https://pvs-studio.com/en/docs/warnings/v591/) Non\-void function should return a value\. main\.c 1010

```cpp
void errx(int, const char *, ...) ;

char *
getoptionvalue(const char *name)
{
  struct option *c;

  if (name == NULL)
    errx(1, "getoptionvalue() invoked with NULL name");
  c = getoption(name);
  if (c != NULL)
    return (c->value);
  errx(1, "getoptionvalue() invoked with unknown option '%s'", name);
  /* NOTREACHED */
}
```

A user's comment NOTREACHED doesn't mean anything here\. You need to annotate functions as noreturn in order to properly write code for such scenarios\. To do this, there are noreturn attributes: standard and compiler\-specific\. First of all, these attributes are taken into account by compilers for proper code generation or notification of certain types of bugs using warnings\. Various static analysis tools also take into account attributes to improve the analysis quality\.

## Handling exceptions

[V596](https://pvs-studio.com/en/docs/warnings/v596/) The object was created but it is not being used\. The 'throw' keyword could be missing: throw ParseException\(FOO\); Response\.cpp 659

```cpp
size_t
Response::ExtractNumber(BDataIO& stream)
{
  BString string = ExtractString(stream);

  const char* end;
  size_t number = strtoul(string.String(), (char**)&end, 10);
  if (end == NULL || end[0] != '\0')
    ParseException("Invalid number!");

  return number;
}
```

The keyword _throw_ was accidentally forgotten here\. Thus, the _ParseException_ exception won't be generated while the object of this class will be simply destroyed when exiting the scope\. After that, the function will continue working as if nothing happened, as if the correct number had been entered\.

[V1022](https://pvs-studio.com/en/docs/warnings/v1022/) An exception was thrown by pointer\. Consider throwing it by value instead\. gensyscallinfos\.cpp 316

```cpp
int
main(int argc, char** argv)
{
  try {
    return Main().Run(argc, argv);
  } catch (Exception& exception) {                                         // <=
    fprintf(stderr, "%s\n", exception.what());
    return 1;
  }
}

int Run(int argc, char** argv)
{
  ....
  _ParseSyscalls(argv[1]);
  ....
}

void _ParseSyscalls(const char* filename)
{
  ifstream file(filename, ifstream::in);
  if (!file.is_open())
    throw new IOException(string("Failed to open '") + filename + "'.");   // <=
  ....
}
```

The analyzer detected the _IOException _exception thrown by pointer\. Throwing a pointer leads to the fact that the exception won't be caught\. So the exception is eventually caught by reference\. In addition, usage of a pointer forces the catching side to call the _delete_ operator to destroy the created object, which hadn't been done\.

A couple of other fragments of code with issues:

* V1022 An exception was thrown by pointer\. Consider throwing it by value instead\. gensyscallinfos\.cpp 347
* V1022 An exception was thrown by pointer\. Consider throwing it by value instead\. gensyscallinfos\.cpp 413

## Formal security

[V597](https://pvs-studio.com/en/docs/warnings/v597/) The compiler could delete the 'memset' function call, which is used to flush 'f\_key' object\. The memset\_s\(\) function should be used to erase the private data\. dst\_api\.c 1018

```cpp
#ifndef SAFE_FREE
#define SAFE_FREE(a) \
do{if(a != NULL){memset(a,0, sizeof(*a)); free(a); a=NULL;}} while (0)
....
#endif

DST_KEY *
dst_free_key(DST_KEY *f_key)
{
  if (f_key == NULL)
    return (f_key);
  if (f_key->dk_func && f_key->dk_func->destroy)
    f_key->dk_KEY_struct =
      f_key->dk_func->destroy(f_key->dk_KEY_struct);
  else {
    EREPORT(("dst_free_key(): Unknown key alg %d\n",
       f_key->dk_alg));
  }
  if (f_key->dk_KEY_struct) {
    free(f_key->dk_KEY_struct);
    f_key->dk_KEY_struct = NULL;
  }
  if (f_key->dk_key_name)
    SAFE_FREE(f_key->dk_key_name);
  SAFE_FREE(f_key);
  return (NULL);
}
```

The analyzer has detected suspicious code, meant for secure private data clearing\. Unfortunately, the _SAFE\_FREE _macro which expands into the _memset_, _free _calls and _NULL _assignment doesn't make the code safer, as it's all removed by the compiler right when optimizing with _O2_\.

By the way, it is nothing else but [CWE\-14](https://cwe.mitre.org/data/definitions/14.html): Compiler Removal of Code to Clear Buffers\.

Here is the list of places, where clearing of buffers is not performed in fact:

* V597 The compiler could delete the 'memset' function call, which is used to flush 'encoded\_block' buffer\. The memset\_s\(\) function should be used to erase the private data\. dst\_api\.c 446
* V597 The compiler could delete the 'memset' function call, which is used to flush 'key\_st' object\. The memset\_s\(\) function should be used to erase the private data\. dst\_api\.c 685
* V597 The compiler could delete the 'memset' function call, which is used to flush 'in\_buff' buffer\. The memset\_s\(\) function should be used to erase the private data\. dst\_api\.c 916
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ce' object\. The memset\_s\(\) function should be used to erase the private data\. fs\_cache\.c 1078

## Comparisons with unsigned variables

[V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'remaining < 0' is always false\. Unsigned type value is never < 0\. DwarfFile\.cpp 1947

```cpp
status_t
DwarfFile::_UnwindCallFrame(....)
{
  ....
  uint64 remaining = lengthOffset + length - dataReader.Offset();
  if (remaining < 0)
    return B_BAD_DATA;
  ....
}
```

The analyzer found an explicit comparison of the unsigned variable with negative values\. Perhaps, one should compare the _remaining_ variable only with null or implement a check for overflow\.

[V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'sleep\(\(unsigned\) secs\) < 0' is always false\. Unsigned type value is never < 0\. misc\.cpp 56

```cpp
status_t
snooze(bigtime_t amount)
{
  if (amount <= 0)
    return B_OK;

  int64 secs = amount / 1000000LL;
  int64 usecs = amount % 1000000LL;
  if (secs > 0) {
    if (sleep((unsigned)secs) < 0)     // <=
      return errno;
  }

  if (usecs > 0) {
    if (usleep((useconds_t)usecs) < 0)
      return errno;
  }

  return B_OK;
}
```

To get the main point of the error, let's address signatures of _sleep _and _usleep_ functions:

```cpp
extern unsigned int sleep (unsigned int __seconds);
extern int usleep (__useconds_t __useconds);
```

As we can see, the _sleep_ function returns the unsigned value and its usage in the code is incorrect\.

## Dangerous pointers

[V774](https://pvs-studio.com/en/docs/warnings/v774/) The 'device' pointer was used after the memory was released\. xhci\.cpp 1572

```cpp
void
XHCI::FreeDevice(Device *device)
{
  uint8 slot = fPortSlots[device->HubPort()];
  TRACE("FreeDevice() port %d slot %d\n", device->HubPort(), slot);

  // Delete the device first, so it cleans up its pipes and tells us
  // what we need to destroy before we tear down our internal state.
  delete device;

  DisableSlot(slot);
  fDcba->baseAddress[slot] = 0;
  fPortSlots[device->HubPort()] = 0;            // <=
  delete_area(fDevices[slot].trb_area);
  delete_area(fDevices[slot].input_ctx_area);
  delete_area(fDevices[slot].device_ctx_area);

  memset(&fDevices[slot], 0, sizeof(xhci_device));
  fDevices[slot].state = XHCI_STATE_DISABLED;
}
```

A _device_ object is_ _freed by the _delete_ operator\. It's quite logical for the _FreeDevice_ function\. But, for some reason, to release other resources, the already removed object is addressed\.

Such code is extremely dangerous and can be met in other several places:

* V774 The 'self' pointer was used after the memory was released\. TranslatorRoster\.cpp 884
* V774 The 'string' pointer was used after the memory was released\. RemoteView\.cpp 1269
* V774 The 'bs' pointer was used after the memory was released\. mkntfs\.c 4291
* V774 The 'bs' pointer was used after the memory was released\. mkntfs\.c 4308
* V774 The 'al' pointer was used after the memory was reallocated\. inode\.c 1155

[V522](https://pvs-studio.com/en/docs/warnings/v522/) Dereferencing of the null pointer 'data' might take place\. The null pointer is passed into 'malo\_hal\_send\_helper' function\. Inspect the third argument\. Check lines: 350, 394\. if\_malohal\.c 350

```cpp
static int
malo_hal_fwload_helper(struct malo_hal *mh, char *helper)
{
  ....
  /* tell the card we're done and... */
  error = malo_hal_send_helper(mh, 0, NULL, 0, MALO_NOWAIT); // <= NULL
  ....
}

static int
malo_hal_send_helper(struct malo_hal *mh, int bsize,
    const void *data, size_t dsize, int waitfor)
{
  mh->mh_cmdbuf[0] = htole16(MALO_HOSTCMD_CODE_DNLD);
  mh->mh_cmdbuf[1] = htole16(bsize);
  memcpy(&mh->mh_cmdbuf[4], data , dsize);                   // <= data
  ....
}
```

Interprocedural analysis revealed the case when _NULL_ is passed to the function and the _data_ pointer with such a value is eventually dereferenced in the _memcpy_ function\.

[V773](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'inputFileFile' pointer\. A memory leak is possible\. command\_recompress\.cpp 119

```cpp
int
command_recompress(int argc, const char* const* argv)
{
  ....
  BFile* inputFileFile = new BFile;
  error = inputFileFile->SetTo(inputPackageFileName, O_RDONLY);
  if (error != B_OK) {
    fprintf(stderr, "Error: Failed to open input file \"%s\": %s\n",
      inputPackageFileName, strerror(error));
    return 1;
  }
  inputFile = inputFileFile;
  ....
}
```

[PVS\-Studio can detect memory leaks](https://pvs-studio.com/en/blog/posts/cpp/0543/)\. In this example, in case of an error the memory won't be released\. Someone might think that in case of errors you shouldn't bother about the memory release, as the program will still end\. But it's not always so\. It is a requirement for many programs to handle errors correctly and to continue working\.

[V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'fReply' pointer was utilized before it was verified against nullptr\. Check lines: 49, 52\. ReplyBuilder\.cpp 49

```cpp
RPC::CallbackReply*
ReplyBuilder::Reply()
{
  fReply->Stream().InsertUInt(fStatusPosition, _HaikuErrorToNFS4(fStatus));
  fReply->Stream().InsertUInt(fOpCountPosition, fOpCount);

  if (fReply == NULL || fReply->Stream().Error() == B_OK)
    return fReply;
  else
    return NULL;
}
```

It's a very common mistake to dereference pointers before checking them\. The [V595](https://pvs-studio.com/en/docs/warnings/v595/) diagnostic almost always prevails in the number of warnings in a project\. This code fragment includes dangerous usage of the _fReply_ pointer\.

[V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'mq' pointer was utilized before it was verified against nullptr\. Check lines: 782, 786\. oce\_queue\.c 782

```cpp
static void
oce_mq_free(struct oce_mq *mq)
{
  POCE_SOFTC sc = (POCE_SOFTC) mq->parent;
  struct oce_mbx mbx;
  struct mbx_destroy_common_mq *fwcmd;

  if (!mq)
    return;
  ....
}
```

A similar example\. The _mg_ pointer gets dereferenced several lines earlier than it's checked for null\. There are a lot of similar places in the project\. In some snippets, pointer's usage and check are quite far from each other, so in this article you'll find only a couple of such examples\. Developers are welcome to check out other examples in the full analyzer report\.

## Miscellaneous

[V645](https://pvs-studio.com/en/docs/warnings/v645/) The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 101

```cpp
static void
dump_acpi_namespace(acpi_ns_device_info *device, char *root, int indenting)
{
  char output[320];
  char tabs[255] = "";
  ....
  strlcat(tabs, "|--- ", sizeof(tabs));
  ....
  while (....) {
    uint32 type = device->acpi->get_object_type(result);
    snprintf(output, sizeof(output), "%s%s", tabs, result + depth);
    switch(type) {
      case ACPI_TYPE_INTEGER:
        strncat(output, "     INTEGER", sizeof(output));
        break;
      case ACPI_TYPE_STRING:
        strncat(output, "     STRING", sizeof(output));
        break;
      ....
    }
    ....
  }
  ....
}
```

The difference between _strlcat_ and _strncat_ functions is not very obvious for someone who is unfamiliar with the description of these functions\. The _strlcat_ function expects the size of the entire buffer as the third argument while the _strncat_ function \- the size of the free space in a buffer, which requires evaluating a needed value before calling the function\. But developers often forget or don't know about it\. Passing the entire buffer size to the _strncat_ function can lead to buffer overflow, as the function will consider this value as an acceptable number of characters to copy\. The _strlcat_ function doesn't have such a problem\. But you have to pass strings, ending with terminal null so that it worked properly\.

Here is the entire list of dangerous places with strings:

* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 104
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 107
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 110
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 113
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 118
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 119
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 120
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 123
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 126
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 129
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 132
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 135
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 138
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 141
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 144
* V645 The 'strncat' function call could lead to the 'features\_string' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. VirtioDevice\.cpp 283
* V645 The 'strncat' function call could lead to the 'features\_string' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. VirtioDevice\.cpp 284
* V645 The 'strncat' function call could lead to the 'features\_string' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. VirtioDevice\.cpp 285

[V792](https://pvs-studio.com/en/docs/warnings/v792/) The 'SetDecoratorSettings' function located to the right of the operator '\|' will be called regardless of the value of the left operand\. Perhaps, it is better to use '\|\|'\. DesktopListener\.cpp 324

```cpp
class DesktopListener : public DoublyLinkedListLinkImpl<DesktopListener> {
public:
 ....
 virtual bool SetDecoratorSettings(Window* window,
         const BMessage& settings) = 0;
 ....
};

bool
DesktopObservable::SetDecoratorSettings(Window* window,
  const BMessage& settings)
{
  if (fWeAreInvoking)
    return false;
  InvokeGuard invokeGuard(fWeAreInvoking);

  bool changed = false;
  for (DesktopListener* listener = fDesktopListenerList.First();
    listener != NULL; listener = fDesktopListenerList.GetNext(listener))
    changed = changed | listener->SetDecoratorSettings(window, settings);

  return changed;
}
```

Most likely, '\|' and '\|\|' operators were muddled\. This error leads to unnecessary calls of the _SetDecoratorSettings_ function\.

[V627](https://pvs-studio.com/en/docs/warnings/v627/) Consider inspecting the expression\. The argument of sizeof\(\) is the macro which expands to a number\. device\.c 72

```cpp
#define PCI_line_size 0x0c /* (1 byte) cache line size in 32 bit words */

static status_t
wb840_open(const char* name, uint32 flags, void** cookie)
{
  ....
  data->wb_cachesize = gPci->read_pci_config(data->pciInfo->bus,
    data->pciInfo->device, data->pciInfo->function, PCI_line_size,
    sizeof(PCI_line_size)) & 0xff;
  ....
}
```

Passing the _0x0c _value to the _sizeof _operator looks suspicious\. Perhaps, the author should have evaluated the size of an object, for example, _data_\.

[V562](https://pvs-studio.com/en/docs/warnings/v562/) It's odd to compare a bool type value with a value of 18: 0x12 \=\= IsProfessionalSpdif\(\)\. CEchoGals\_mixer\.cpp 533

```cpp
typedef bool BOOL;

virtual BOOL IsProfessionalSpdif() { ... }

#define ECHOSTATUS_DSP_DEAD 0x12

ECHOSTATUS CEchoGals::ProcessMixerFunction(....)
{
  ....
  if ( ECHOSTATUS_DSP_DEAD == IsProfessionalSpdif() ) // <=
  {
    Status = ECHOSTATUS_DSP_DEAD;
  }
  else
  {
    pMixerFunction->Data.bProfSpdif = IsProfessionalSpdif();
  }
  ....
}
```

The _IsProfessionalSpdif _function returns the _bool _type value\. In doing so, the function's result is compared with the number _0x12_ in the condition\.

## Conclusion

We missed the release of the first Haiku beta last fall, as we were busy releasing PVS\-Studio for Java\. Still the nature of programming errors is such that they don't disappear if you don't search for them and don't pay attention to the code quality\. Project developers used Coverity Scan, but the last run was almost two years ago\. This must be upsetting to Haiku users\. Even though the analysis has been configured in 2014 using Coverity, it didn't stop us from writing two long articles on errors review in 2015 \([part 1](https://pvs-studio.com/en/blog/posts/cpp/0317/), [part 2](https://pvs-studio.com/en/blog/posts/cpp/0318/)\)\.

Another Haiku review of errors is coming out soon for those who read this post up to the end\. The full analyzer report will be sent to developers before posting this errors review, so some errors might be fixed by the time you're reading this\. To pass the time between the articles, I suggest downloading and trying [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) for your project\.

Do you want to try Haiku and you have questions? Haiku developers invite you to the [telegram\-channel](https://t.me/haiku_os)\.