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

This story goes back to 2015, when Haiku OS and PVS-Studio static analyzer developers decided to join forces and improve this OS code quality. At first it was more like an experiment, as there was no Linux analyzer at that time and the team had to work only with the compiled executable analyzer file. The entire infrastructure for parsing compiler parameters, running preprocessor, analysis paralleling and so on was taken from the Compiler Monitoring UI utility in C#, which was ported in parts to the Mono platform in order to be run in Linux.

This story goes back to 2015, when Haiku OS and PVS-Studio static analyzer developers decided to join forces and improve this OS code quality. At first it was more like an experiment, as there was no Linux analyzer at that time and the team had to work only with the compiled executable analyzer file. The entire infrastructure for parsing compiler parameters, running preprocessor, analysis paralleling and so on was taken from the Compiler Monitoring UI utility in C#, which was ported in parts to the Mono platform in order to be run in Linux.

Now 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 OS building and thank Haiku OS developers for their help in building the project.

Interestingly, 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. Haiku developers tried to use Coverity Scan, but, most sadly, the last analysis run was almost two years ago. 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). Now four years later, a new article about checking this project appears.

Note:here will be some interesting errors from the project, a more complete report can be checked out in the article "How to shoot yourself in the foot in C and C++. Haiku OS Cookbook".

So let's move on to the errors:

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.

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.

Errors with the free function

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 [] 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.

Follow our PVS-Studio team blog to see another Haiku OS errors review coming out soon for those who read the first part 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.