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.

Introduction

Meet the main characters of our story — the Haiku with open source code and the PVS-Studio 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, 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:

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. This can make it very easy for some companies to apply static analysis techniques for their projects.

Uninitialized variables

V614 Uninitialized variable 'rval' used. fetch.c 1727

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 variablecan become a return value of the auto_fetch function.

V614 Uninitialized pointer 'res' used. commands.c 2873

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 Pointer to local variable 'normalized' is stored outside the scope of this variable. Such a pointer will become invalid. TextView.cpp 5596

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

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

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 The 'then' statement is equivalent to the 'else' statement. subr_gtaskqueue.c 191

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

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:

/* 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 The null pointer is passed into 'free' function. Inspect the first argument. setmime.cpp 727

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 The null pointer is passed into 'free' function. Inspect the first argument. driver_settings.cpp 461

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 The null pointer is passed into 'free' function. Inspect the first argument. PackageFileHeapWriter.cpp 166

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

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

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:

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 classwon't be called due to the use of the free function to release memory.

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

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 Calling a 'delete' operator for a void pointer will cause undefined behavior. Hashtable.cpp 207

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 Non-void function should return a value. Referenceable.h 228

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 Non-void function should return a value. main.c 1010

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 The object was created but it is not being used. The 'throw' keyword could be missing: throw ParseException(FOO); Response.cpp 659

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 An exception was thrown by pointer. Consider throwing it by value instead. gensyscallinfos.cpp 316

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

#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: 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 Expression 'remaining < 0' is always false. Unsigned type value is never < 0. DwarfFile.cpp 1947

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 Expression 'sleep((unsigned) secs) < 0' is always false. Unsigned type value is never < 0. misc.cpp 56

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:

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 The 'device' pointer was used after the memory was released. xhci.cpp 1572

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 isfreed 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 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

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 The function was exited without releasing the 'inputFileFile' pointer. A memory leak is possible. command_recompress.cpp 119

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. 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 The 'fReply' pointer was utilized before it was verified against nullptr. Check lines: 49, 52. ReplyBuilder.cpp 49

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 diagnostic almost always prevails in the number of warnings in a project. This code fragment includes dangerous usage of the fReply pointer.

V595 The 'mq' pointer was utilized before it was verified against nullptr. Check lines: 782, 786. oce_queue.c 782

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

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

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 Consider inspecting the expression. The argument of sizeof() is the macro which expands to a number. device.c 72

#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 It's odd to compare a bool type value with a value of 18: 0x12 == IsProfessionalSpdif(). CEchoGals_mixer.cpp 533

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, part 2)

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 for your project.

Do you want to try Haiku and you have questions? Haiku developers invite you to the telegram-channel.