Skip to Content
Course content

133: Static Analysis Tools for C++

Click on the "Edit" button in the top corner of the screen to edit your slide content.

By now, you've probably gotten used to the compiler shouting at you when you miss a semicolon or try to add a string to an integer. But as your projects grow, you'll find that the compiler is often too permissive. It tells you if the code is legal, but it rarely tells you if the code is stupid. That's where static analysis comes in.

Static analysis tools scan your source code without actually running it. They look for patterns that historically lead to crashes, memory leaks, or "undefined behavior" that might only trigger once every thousand runs in production. I've spent way too many late nights debugging heisenbugs that a simple run of clang-tidy or cppcheck would have caught in three seconds.

Writing a Fragile DataBuffer

To show you how this works, let's build a small DataBuffer class. I want this class to manage a raw heap array—not because this is the best way to do it in modern C++ (we have std::vector for a reason), but because raw pointers are where static analysis tools really shine.

#include <iostream>
#include <algorithm>

class DataBuffer {
    int* data;
    size_t size;

public:
    DataBuffer(size_t s) {
        size = s;
        data = new int[size];
    }

    ~DataBuffer() {
        delete[] data;
    }

    void set(size_t index, int value) {
        data[index] = value;
    }
};

At first glance, this looks fine. But if I run a static analyzer like clang-tidy against this, it's going to start complaining immediately. The most glaring issue it'll flag is the "Rule of Three." I've defined a destructor, but I haven't defined a copy constructor or a copy assignment operator. If you copy this object, you get two objects pointing to the same memory, and when they both go out of scope, you get a double-free crash.

Tightening the Screws with Clang-Tidy

I'll start by fixing the copy behavior. I'll delete the copy operations to make the class "move-only," which is a common pattern for resource handles. While I'm at it, I'll initialize my members in the constructor initialization list rather than the body—another thing static analyzers love to nag you about.

class DataBuffer {
    int* data;
    size_t size;

public:
    DataBuffer(size_t s) : data(new int[s]), size(s) {}

    ~DataBuffer() {
        delete[] data;
    }

    // Prevent accidental copying
    DataBuffer(const DataBuffer&) = delete;
    DataBuffer& operator=(const DataBuffer&) = delete;

    // Add move support
    DataBuffer(DataBuffer&& other) noexcept : data(other.data), size(other.size) {
        other.data = nullptr;
        other.size = 0;
    }
};

The "Wait, I Forgot" Moment

Here is where I usually trip up. I've implemented the move constructor, but I completely forgot the move assignment operator. I'll write it quickly, but I'm going to make a classic mistake: I'll forget to delete the existing resource before taking over the new one.

DataBuffer& operator=(DataBuffer&& other) noexcept {
    data = other.data;
    size = other.size;
    other.data = nullptr;
    other.size = 0;
    return *this;
}

If I ran this code, and the DataBuffer already had memory allocated, that memory would be leaked the moment I assigned the new pointer to data. The compiler won't warn me about this because the syntax is perfectly valid C++. However, a static analyzer will flag this as a "resource leak." It recognizes that data was overwritten without being freed.

The fix is simple: I need to delete[] data; before taking the pointer from other. Or, even better, I can use the "copy-and-swap" idiom, though for move-only types, a direct cleanup is often cleaner.

Finalizing the Hardened Class

After running the tools and fixing my leak, the final version looks like this. I've also added a bounds check in the set method, because cppcheck will tell you that accessing an array without checking the index is just asking for a segmentation fault.

#include <iostream>
#include <stdexcept>

class DataBuffer {
    int* data;
    size_t size;

public:
    explicit DataBuffer(size_t s) : data(new int[s]), size(s) {}

    ~DataBuffer() {
        delete[] data;
    }

    DataBuffer(const DataBuffer&) = delete;
    DataBuffer& operator=(const DataBuffer&) = delete;

    DataBuffer(DataBuffer&& other) noexcept : data(other.data), size(other.size) {
        other.data = nullptr;
        other.size = 0;
    }

    DataBuffer& operator=(DataBuffer&& other) noexcept {
        if (this != &other) {
            delete[] data; // Fix: prevent the leak I made earlier
            data = other.data;
            size = other.size;
            other.data = nullptr;
            other.size = 0;
        }
        return *this;
    }

    void set(size_t index, int value) {
        if (index >= size) throw std::out_of_range("Index out of bounds");
        data[index] = value;
    }
};

My advice? Don't wait for the tool to tell you you're wrong. Integrate these into your CI/CD pipeline or your IDE. It's much better to have a tool tell you that you've leaked memory now than to have a customer tell you their server crashed after three days of uptime.




📋 Practical Task

Hardening the PacketProcessor Class

You are given a class called PacketProcessor that is riddled with "code smells" that a static analyzer would catch. Your task is to refactor the code to fix the following issues:

  • A potential memory leak in the assignment operator.
  • A missing virtual destructor in a class intended for inheritance.
  • Uninitialized member variables.
  • A potential null pointer dereference in the process() method.
#include <iostream>
#include <cstring>

class BaseProcessor {
    // Issue 1: Missing something here?
    ~BaseProcessor() {} 
};

class PacketProcessor : public BaseProcessor {
    char* buffer;
    int length; // Issue 2: Uninitialized

public:
    PacketProcessor(const char* input) {
        length = std::strlen(input);
        buffer = new char[length + 1];
        std::strcpy(buffer, input);
    }

    ~PacketProcessor() {
        delete[] buffer;
    }

    PacketProcessor& operator=(const PacketProcessor& other) {
        // Issue 3: Potential leak here
        buffer = new char[other.length + 1];
        std::strcpy(buffer, other.buffer);
        length = other.length;
        return *this;
    }

    void process(char* external_data) {
        // Issue 4: Unsafe access
        std::cout < "Processing: " < external_data[0] < std::endl;
    }
};

Rewrite the PacketProcessor and BaseProcessor classes to be "statically clean." Ensure you handle the memory correctly, add the necessary keywords for inheritance, and add a safety check to the process method.

Rating
0 0

There are no comments for now.

to be the first to leave a comment.