Chapter 45 — The Callers Must Not Notice

Chapter 45 — The Callers Must Not Notice¶

Most of the code you are handed at work already works, has callers in three teams, and must still have them, unchanged, when you are done — a place none of the labs so far started from, and the one the job most often does. The mandate arrives as a ticket, the way it does, and the first thing to notice about it is that the acceptance test is written into the ticket itself.

The ticket¶

Modernise Catalog for 4.0 — without touching the callers. The native layer's Catalog is 2009 code: raw pointers, a hand-rolled array, a manual Clear. The 4.0 snapshot feature takes a copy of a Catalog, and the feature branch dies at exit under the sanitizers — heap-use-after-free in Catalog::Clear. The review board's rule for 4.0 is no owning raw pointers in the native layer. Three teams' files include catalog.h; none of them may change, and none of their behaviour may.

Read the last sentence twice. None of them may change is checkable by a build: compile the same caller against the old implementation and the new one. None of their behaviour may is the half that is not, and is the half this chapter is about.

The code it happened to¶

The header the three teams compiled against, comments included — they are part of what the callers rely on:

#ifndef CATALOG_H
#define CATALOG_H

struct Entry {
    char key[32];
    int  value;
};

class Catalog {
public:
    Catalog();
    ~Catalog();

    // Parses "key=value;key=value;..." into the catalog. Returns the number
    // of entries added, or -1 if the text is malformed (nothing added).
    int Parse(const char* text);

    // Adds or replaces one entry. The catalog keeps its own copy of the key.
    void Add(const char* key, int value);

    // Borrowed: the pointer stays valid until the entry is removed or the
    // catalog is cleared - adding entries does NOT move existing ones.
    const Entry* Find(const char* key) const;

    // C-style: true and *out filled if the key exists.
    bool TryGet(const char* key, int* out) const;

    int  Count() const;
    void Clear();

private:
    void Grow();

    Entry** entries_;
    int     count_;
    int     capacity_;
};

#endif

And the implementation behind it, exactly as it has shipped since 2009:

#include "catalog.h"
#include <cstdlib>
#include <cstring>

Catalog::Catalog() : entries_(0), count_(0), capacity_(0) {}

Catalog::~Catalog() {
    Clear();
    delete[] entries_;
}

void Catalog::Grow() {
    int newCap = capacity_ == 0 ? 4 : capacity_ * 2;
    Entry** bigger = new Entry*[newCap];
    for (int i = 0; i < count_; ++i) bigger[i] = entries_[i];
    delete[] entries_;
    entries_ = bigger;
    capacity_ = newCap;
}

void Catalog::Add(const char* key, int value) {
    for (int i = 0; i < count_; ++i) {
        if (std::strcmp(entries_[i]->key, key) == 0) {
            entries_[i]->value = value;      // replace in place: the address stays
            return;
        }
    }
    if (count_ == capacity_) Grow();
    Entry* e = new Entry;
    std::strncpy(e->key, key, sizeof e->key - 1);
    e->key[sizeof e->key - 1] = '\0';
    e->value = value;
    entries_[count_++] = e;
}

int Catalog::Parse(const char* text) {
    // Validate first, so a malformed text adds nothing.
    for (const char* p = text; *p; ) {
        const char* eq = std::strchr(p, '=');
        if (!eq || eq == p || eq - p >= 32) return -1;
        char* end;
        std::strtol(eq + 1, &end, 10);
        if (end == eq + 1 || (*end != ';' && *end != '\0')) return -1;
        p = *end == ';' ? end + 1 : end;
    }
    int added = 0;
    for (const char* p = text; *p; ) {
        const char* eq = std::strchr(p, '=');
        char key[32];
        std::memcpy(key, p, eq - p);
        key[eq - p] = '\0';
        char* end;
        long v = std::strtol(eq + 1, &end, 10);
        Add(key, static_cast<int>(v));
        ++added;
        p = *end == ';' ? end + 1 : end;
    }
    return added;
}

const Entry* Catalog::Find(const char* key) const {
    for (int i = 0; i < count_; ++i) {
        if (std::strcmp(entries_[i]->key, key) == 0) return entries_[i];
    }
    return 0;
}

bool Catalog::TryGet(const char* key, int* out) const {
    const Entry* e = Find(key);
    if (!e) return false;
    *out = e->value;
    return true;
}

int Catalog::Count() const { return count_; }

void Catalog::Clear() {
    for (int i = 0; i < count_; ++i) delete entries_[i];
    count_ = 0;
}

A caller, as one of the three teams wrote it — this file is the acceptance test's other half, and it will not change by a byte between here and the end of the chapter:

#include "catalog.h"
#include <cstdio>

static void Report(const Catalog* c, const char* key) {
    int v = 0;
    if (c->TryGet(key, &v)) {
        std::printf("%s = %d\n", key, v);
    } else {
        std::printf("%s: not found\n", key);
    }
}

int main() {
    Catalog c;
    int n = c.Parse("alpha=1;beta=2;gamma=3");
    std::printf("parsed %d, count %d\n", n, c.Count());

    const Entry* alpha = c.Find("alpha");     // borrowed, held across Adds
    for (int i = 0; i < 10; ++i) {
        char key[16];
        std::snprintf(key, sizeof key, "k%d", i);
        c.Add(key, i * 10);
    }
    std::printf("alpha still at %s = %d, count %d\n", alpha->key, alpha->value, c.Count());

    c.Add("beta", 22);                        // replace in place
    Report(&c, "beta");
    Report(&c, "delta");
    std::printf("malformed: %d, count %d\n", c.Parse("x=;"), c.Count());

    c.Clear();
    std::printf("cleared: count %d\n", c.Count());
    return 0;
}

It works. Under the full canonical flags it is green, it prints six lines, and it has done so for fifteen years. Nothing in the ticket is a bug in this code — until the snapshot feature writes the one line nobody had:

Catalog snapshot = live;                  // the feature: a copy to compare against later

Against the 2009 class, on this machine, the branch dies at the end of main with the report the ticket quoted:

==91837==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000001c60 ...
READ of size 8 at 0x603000001c60 thread T0
    #0 in Catalog::Clear() catalog.cpp:82
    #1 in Catalog::~Catalog() catalog.cpp:12
    ...
    #3 in main snapshot.cpp:61

freed by thread T0 here:
    #0 in _ZdaPv (libclang_rt.asan_osx_dynamic.dylib)
    #1 in Catalog::Grow() catalog.cpp:20
    #2 in Catalog::Add(char const*, int) catalog.cpp:32
    #3 in main snapshot.cpp:38

previously allocated by thread T0 here:
    #0 in _Znam (libclang_rt.asan_osx_dynamic.dylib)
    #1 in Catalog::Grow() catalog.cpp:18
    #2 in Catalog::Add(char const*, int) catalog.cpp:32
    #3 in Catalog::Parse(char const*) catalog.cpp:58

SUMMARY: AddressSanitizer: heap-use-after-free catalog.cpp:82 in Catalog::Clear()

Try it — before reading on¶

The task card is exercises/retrolab/TASK.md; before/ beside it is the 2009 class, main.cpp the caller that may not change, snapshot.cpp the 4.0 feature with its own judge, and after/ the retrofit — no peeking at the last. The ticket is worked one seam at a time, and every seam has the same test:

Read the header for the promises it makes, in its comments as much as its declarations: what Find says about the address it returns, what Parse says about malformed input, what Clear is for. Write them down. That list, not the declarations, is the contract you are about to keep — and the part of it the compiler cannot check.
Reproduce. The caller against before/, green; snapshot.cpp against before/, the report above. Read it against Chapter 6: which special member is on the access stack, who wrote it, and what did the freed by stack's Grow do to the other catalog?
Seam 1 — declare what the compiler was writing for you. Two lines: the copy operations, = delete. Rebuild every caller; then rebuild the snapshot branch. One of those outcomes is the point.
Seam 2 — move the ownership inside. Before you choose the container, predict what std::vector<Entry> would do to the caller's alpha, then try it, and run the byte-identical caller. Then choose again.
Seam 3 — let the destructor go. = default, with Clear still public because callers call it. Say why this seam comes after seam 2 and not before.
Seam 4 — earn the copy. Deep copy, move, copy-and-swap. The snapshot branch compiles, and its judge passes. At every seam: the caller against your current state, its output compared byte for byte with the 2009 output, under the full flags.
Stretch: the boundary. Suppose catalog.h shipped in an SDK and the three teams' binaries could not be rebuilt. Which of the four seams are still allowed? Take the answer to Chapter 30 and check it.

The diagnosis, walked through¶

Show the walkthrough — read the report against Chapter 6 first

The class has two special members it never declared — the copy constructor and copy assignment — and in 2009 the compiler wrote both, member-wise, which for Entry** entries_ means copy the pointer. Nobody copied a Catalog for fifteen years, so the shallow copy was never wrong; it was never run. The snapshot feature runs it: two Catalog objects, one entries_ array between them. Then the live catalog grows — Grow allocates a bigger array and deletes the old one, which is the freed by stack — and from that line the snapshot's entries_ points at freed memory. Its destructor's Clear walks it, and that is the access stack. Chapter 6's Rule of Five — its C++03 half, the Rule of Three — violated by omission in 2009, invoiced in 4.0: a class that owns a resource and does not say what a copy means gets a copy that means the wrong thing, on the day someone finally writes one.

Read what the report does not say. Nothing in it is in snapshot.cpp's copy line; the crime happened on line 38, in Add, where a perfectly ordinary growth freed something a second object still believed it owned. The report names the victim and the freer and never the moment of the shared ownership, because that moment was a compiler-generated function with no source line of its own. That is what "declare what the compiler was writing for you" means: seam 1 gives that function a line, and the line says = delete.

What the contract actually says¶

A name for the mandate: the retrofit — changing the insides of a class whose surface is frozen because other people compiled against it. The surface is more than the declarations. It is everything a caller can observe: the declarations, yes, and so the same const char* parameters and int returns and the public Clear; but also the promises written in the header's comments, and the ones nobody wrote down because the old implementation simply had them. Find returns a pointer that stays valid while the catalog grows. Parse adds nothing when the text is malformed. A Catalog with nothing in it is a valid Catalog. None of those appears in a signature, and every one is something a caller somewhere depends on.

Surprise for C# devs: you have done this refactor many times — an interface or a public surface held still, the class behind it rewritten — and the compiler held the surface for you, because in C# a caller can observe almost nothing else. Here a caller can observe the address of what you hand back, the order in which things are destroyed, and which of copy and move the compiler generated for you. Source that still compiles is a necessary condition. It is not behaviour that still holds, and no keyword says so.

That is why the acceptance test has two halves. The first is a build: the caller's translation unit, byte-identical, compiled against the old implementation and the new one. The second is a run: the two binaries' output, byte-identical, under the full flags — because the byte-identical caller is exactly the one that will die with a use-after-free the moment a seam breaks a promise nobody wrote down. Seam 2 is where that happens. Modernise the storage to std::vector<Entry> — the obvious move, one type, no pointers anywhere — and the caller, unchanged, prints its first line and then:

==91866==ERROR: AddressSanitizer: heap-use-after-free on address 0x60d000000060 ...
READ of size 4 at 0x60d000000060 thread T0
    #0 in main main.cpp:30

freed by thread T0 here:
    #0 in _ZdlPv (libclang_rt.asan_osx_dynamic.dylib)
    #1 in std::__libcpp_deallocate<Entry>(...) allocate.h:81
    #2 in std::allocator<Entry>::deallocate(Entry*, unsigned long) allocator.h:120
    ...

SUMMARY: AddressSanitizer: heap-use-after-free main.cpp:30 in main

The caller held alpha across ten Adds, as the header told it it could, and the vector moved every Entry on the second one. The caller did not change. Its behaviour did — which is the sentence the ticket ends on, and the reason the container is std::vector<std::unique_ptr<Entry>>: the vector may move, the entries do not, and the 2009 promise survives a change of storage it was never told about.

And one boundary the retrofit does not cross. Everything above assumes the three teams recompile — source compatibility. The day catalog.h ships in an SDK and the callers' binaries cannot be rebuilt, sizeof(Catalog) is part of the contract too, and the seam that moves the storage changes it — the one every later seam is built on. That is Chapter 30's subject, and its answer is the one seam that never moves again: a pointer to an implementation, behind which this whole chapter can happen without a caller relinking.

The fix, seam by seam¶

Four seams, each a commit, each green against the unchanged caller before the next begins. That discipline is the deliverable: not the final class, which any of the earlier chapters could have written from scratch, but the path to it through states that never break the three teams.

Seam 1 — declare what the compiler was writing for you. Two lines in the header, and nothing else:

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

Every caller still compiles, because none of them copied — that is the build proving the claim rather than the author asserting it. The snapshot branch now fails to compile, at the copy line, with a message that names the deleted function: a crash at exit on a customer's machine has become a compile error on the developer's, which is the largest single improvement in the whole ticket and it cost two lines. Chapter 6's advice — if you cannot yet say what a copy means, forbid it — applied to code that had been silently permitting one for fifteen years.

Seam 2 — move the ownership inside, keeping the address promise. The private section changes, the public one does not:

void Catalog::Add(const char* key, int value) {
    for (auto& e : entries_) {
        if (std::strcmp(e->key, key) == 0) {
            e->value = value;                // replace in place: the address stays
            return;
        }
    }
    auto e = std::make_unique<Entry>();
    std::strncpy(e->key, key, sizeof e->key - 1);
    e->key[sizeof e->key - 1] = '\0';
    e->value = value;
    entries_.push_back(std::move(e));        // the vector may move: the Entry does not
}

const Entry* Catalog::Find(const char* key) const {
    for (const auto& e : entries_) {
        if (std::strcmp(e->key, key) == 0) return e.get();
    }
    return nullptr;
}

Grow is gone — the vector grows itself — and Find returns e.get(), a borrowed pointer into an Entry that the vector's growth never moves. That is the whole reason for the unique_ptr per entry rather than the entry by value, and it is a decision made for the caller rather than for the class.

Seam 3 — let the destructor go. ~Catalog() = default; — after seam 2, never before it: a defaulted destructor over a raw Entry** is a leak of every entry, and the sanitizers on this platform would not have said so (Chapter 31's macOS note). Clear() stays public and stays a member, because callers call it; it just is not the destructor's job any more.

Seam 4 — earn the copy. Now that the storage owns, a correct copy is two short functions, and the move operations are the compiler's for the asking:

// Seam 4: copy is a copy of the ENTRIES, one allocation each, so the two
// catalogs share nothing; move steals the vector and leaves the source
// empty, which is a valid Catalog (Count() == 0). Copy-and-swap makes
// assignment self-safe and exception-safe for free (Chapter 6).
Catalog::Catalog(const Catalog& other) {
    entries_.reserve(other.entries_.size());
    for (const auto& e : other.entries_) {
        entries_.push_back(std::make_unique<Entry>(*e));
    }
}

Catalog& Catalog::operator=(const Catalog& other) {
    Catalog copy(other);
    entries_.swap(copy.entries_);
    return *this;
}

Catalog::Catalog(Catalog&& other) noexcept = default;
Catalog& Catalog::operator=(Catalog&& other) noexcept = default;

The header after all four, every public declaration where it was and the special members it always needed finally declared:

#ifndef CATALOG_H
#define CATALOG_H

#include <memory>
#include <vector>

struct Entry {
    char key[32];
    int  value;
};

class Catalog {
public:
    Catalog();
    ~Catalog();

    // Seam 4: the copy the class silently had was a shallow one, which is
    // why nobody was allowed to copy a Catalog. Now it is a deep one, and
    // the snapshot feature can.
    Catalog(const Catalog& other);
    Catalog& operator=(const Catalog& other);
    Catalog(Catalog&& other) noexcept;
    Catalog& operator=(Catalog&& other) noexcept;

    // Parses "key=value;key=value;..." into the catalog. Returns the number
    // of entries added, or -1 if the text is malformed (nothing added).
    int Parse(const char* text);

    // Adds or replaces one entry. The catalog keeps its own copy of the key.
    void Add(const char* key, int value);

    // Borrowed: the pointer stays valid until the entry is removed or the
    // catalog is cleared - adding entries does NOT move existing ones.
    const Entry* Find(const char* key) const;

    // C-style: true and *out filled if the key exists.
    bool TryGet(const char* key, int* out) const;

    int  Count() const;
    void Clear();

private:
    // Seam 2: the storage owns. One unique_ptr per entry, not a vector of
    // entries, because Find's promise - the address stays put while the
    // catalog grows - is part of the contract, and a vector<Entry> would
    // break every caller that holds a pointer across an Add without
    // changing a line of their code.
    std::vector<std::unique_ptr<Entry>> entries_;
};

#endif

The judge for the feature, and for the retrofit's own promises — a copy that shares nothing, a move that leaves a valid empty source, a self-assignment that changes nothing, and the address promise carried across the change of storage:

#include "catalog.h"
#include <cstdio>
#include <utility>

namespace {
int g_failures = 0;
void Check(bool ok, const char* what) {
    if (!ok) {
        std::printf("FAILED: %s\n", what);
        ++g_failures;
    }
}
}   // namespace

int main() {
    Catalog live;
    live.Parse("alpha=1;beta=2");

    Catalog snapshot = live;                  // the feature: a copy to compare against later
    live.Add("alpha", 100);
    live.Add("gamma", 3);
    int v = 0;
    Check(snapshot.TryGet("alpha", &v) && v == 1, "the snapshot kept the old value");
    Check(!snapshot.TryGet("gamma", &v), "the snapshot did not gain the new key");
    Check(live.Count() == 3 && snapshot.Count() == 2, "the two catalogs are independent");

    const Entry* alpha = live.Find("alpha");
    for (int i = 0; i < 100; ++i) {
        char key[16];
        std::snprintf(key, sizeof key, "k%d", i);
        live.Add(key, i);
    }
    Check(live.Find("alpha") == alpha && alpha->value == 100, "Find's address survived a hundred Adds - the 2009 promise, kept");

    Catalog moved = std::move(live);
    Check(moved.Count() == 103 && live.Count() == 0, "a move leaves the source valid and empty");
    live.Add("again", 1);
    Check(live.Count() == 1, "and usable");

    Catalog assigned;
    assigned = std::move(moved);              // move ASSIGNMENT, which a move constructor check does not cover
    Check(assigned.Count() == 103 && moved.Count() == 0, "a move assignment steals the same way");

    Catalog* self = &assigned;
    assigned = *self;                         // self-assignment through a pointer, so the compiler cannot see it
    Check(assigned.Count() == 103, "self-assignment changes nothing");

    if (g_failures != 0) {
        std::printf("retrolab: %d FAILED\n", g_failures);
        return 1;
    }
    std::printf("retrolab: the snapshot feature holds, and the address promise with it\n");
    return 0;
}

build_all.sh runs the acceptance test as the ticket wrote it: the caller above, unchanged, built against before/ and against after/, both under the full flags, and the two outputs compared byte for byte; then the snapshot judge against after/. The caller's file being identical is true by construction. The caller's output being identical is the claim, and it is the one the value-storage seam would have failed.

Pitfalls¶

Modernising the storage to values. std::vector<Entry> is the cleanest type and the wrong retrofit: the 2009 promise was address stability, and every caller holding a pointer across an Add dies without a line of theirs changing. The container that keeps a promise nobody wrote down is chosen by reading the callers, not the class.
Defaulting the destructor before the storage owns. A = default over a raw Entry** leaks every entry, silently on macOS. Seams have an order: ownership moves inside first, then the manual cleanup goes.
Improving the surface while you are in there. const char* to std::string_view, int to size_t, Find to std::optional — each a better API, each a change to what three teams compiled against, each out of scope by the ticket's first sentence. Write them on the list for 5.0 and put nothing in the header a caller can see.
Adding an include to the header that callers did not have. <vector> and <memory> now reach three teams' translation units. Usually harmless; occasionally a name collision or a build-time change they will notice. It is a change to the surface, however small, and it goes in the commit message.
One big rewrite. The end state is easy; the ticket is the path. A seam is the unit — small enough that when the byte-identical caller's output changes, there is one thing it could have been.
Stopping at "it compiles". Source compatibility is what the compiler checks. The caller's output under the sanitizers is what the ticket asked for, and the difference between the two is a customer's crash report with your name on the retrofit commit.

Key principle: "I modernise a working class one seam at a time — declare what the compiler was writing for me, move the ownership inside, then earn the copy — with the callers' files untouched and their output byte-identical at every seam, because source that still compiles is not behaviour that still holds."

In the wild¶

The rule-of-zero retrofit is the commonest engineering task in a codebase that predates C++11, and the industry's tooling grew around exactly the seams above. The standard has deprecated generating a copy for a class with a user-declared destructor since C++11, and clang's -Wdeprecated-copy-with-dtor (GCC: -Wdeprecated-copy-dtor) says so — seam 1 as a diagnostic, and outside -Wall -Wextra, which is why the 2009 class is green under the canonical flags. clang-tidy's cppcoreguidelines-owning-memory names every owning raw pointer for you — seam 2's inventory, mechanically, and without knowing which addresses your callers hold. LLVM's and Chromium's C++11 migrations were incremental and gated on a build of every caller; where Chromium rewrote by tool, the tool ran over the whole tree in one change, because a build of every caller was the only acceptance test there was — "callers unchanged" is a claim a build makes, at any scale. And the boundary the chapter stops at is where a whole ecosystem lives: Qt's d-pointer is Chapter 30's PIMPL applied to every public class so that its insides can change for the life of a major series — a binary built against 5.0 in 2012 still loads against 5.15, and each new major is the one place the ABI is allowed to break — which is the retrofit at the scale of a framework, and the reason that, when the teams cannot recompile, the first seam is the one that moves everything behind a pointer and the rest of this chapter happens on the far side of it.