Collaborative Development and Code Review

1Branching

git switch -c fix-config-leak      # branch and switch (modern spelling)
# … work, commit …
git push -u origin fix-config-leak

One branch per change, named after the change. Keep it short-lived: a branch open for three weeks diverges until merging it is its own project.

Merge or rebase

git switch fix-config-leak
git fetch origin
git merge origin/main             # preserves history; adds a merge commit
# or
git rebase origin/main            # replays your commits; linear history

The workable rule: rebase your own unpushed work, merge everything else. Rebasing commits that someone else has pulled rewrites history they already have, and the recovery is unpleasant.

Before submitting, tidy your own branch:

git rebase -i origin/main         # squash "fix typo" into its parent
git log --oneline origin/main..   # review what you are about to submit

Nobody needs to see "wip", "fix typo", and "actually fix it". They need to see one coherent commit per idea.

2Submitting a change

A pull request has three parts a reviewer reads in order: the title, the description, and the diff. Most of the review's quality is decided before the diff.

Title:  Fix use-after-free when config reload fails

What
  parse_config() freed the old configuration before validating the
  new one. A malformed file left the global pointer dangling and the
  next request dereferenced freed memory.

Why this fix
  Validate into a temporary, then swap and free. The alternative —
  copying the old config first — doubles peak memory for a case that
  should be rare.

Testing
  New test reloads with a syntax error and checks the old config is
  still served. Full suite passes under ASan and UBSan.

Risk
  The swap is not atomic; a concurrent reader could observe either
  version. That was already true and is out of scope here (#1240).

The Risk section is the one people omit and reviewers value most. Stating what you did not fix, and why, prevents the review spending its attention rediscovering it.

3Reviewing C

Read the diff twice. Once for what it does, once against this list — in order, because the early items cause the expensive failures.

Memory

• Every allocation checked for NULL?
• Every path — including every early return — frees what it allocated?
• Any pointer used after free? Set to NULL afterwards?
• realloc assigned to a temporary, not to the original pointer?
• Ownership stated: who frees the result, and is it documented?

Bounds

• Every array index provably within range?
• Every buffer write bounded, with the size passed alongside the pointer?
• strcpy, strcat, sprintf, or gets anywhere? Each is a finding.
• Room for the terminator — length + 1?
• Loop bounds < rather than <=?

Integers and undefined behavior

• Signed and unsigned compared or mixed in arithmetic?
• A size computed by multiplication that could overflow?
• An overflow check performed after the operation?
• Shift counts bounded by the type's width?
• Any variable read before it is written?

Errors and concurrency

• Every return value that can fail actually checked?
• errno saved before any intervening call?
• Shared state accessed without a lock? Locks taken in a consistent order?
• Any non-reentrant function — strtok, localtime — in threaded code?

4How to write a review comment

Review the code, never the person. Two formulations of the same finding:

Poor:  You forgot to free buf on the error path.

Good:  Line 47: if parse() fails we return without freeing buf.
       A goto cleanup here would match the pattern used in
       load_index() and cover both exits.

The second names the location, states the consequence, proposes a fix, and cites a precedent in the same codebase. It is also shorter to act on.

Mark the severity, because not everything is equal:

Without the labels, a reviewer's typo comment and their use-after-free comment look identical, and the author has to guess which blocks the merge.

Two further habits: approve when it is good enough rather than perfect — a review that demands perfection stalls everything — and say what you liked, because a review consisting only of complaints teaches nothing about what to repeat.

5Versions and changelogs

Semantic versioning — MAJOR.MINOR.PATCH — states what a consumer must do:

For a C library the rule is stricter than for most languages, because compatibility has two levels. API compatibility means existing source still compiles. ABI compatibility means an already-compiled program still links and runs — and that breaks on changes invisible in the header's text, such as adding a member to a struct the caller can see, or reordering enumerators. Week 45 covers the mechanics; here, note only that "I just added a field" can be a major version.

A changelog is written for users, not from git log:

## 2.3.0 — 2026-09-20

### Added
- `config_reload()` for reloading without a restart

### Fixed
- Use-after-free when a config reload failed (#1234)
- Memory leak in the error path of `index_open()` (#1251)

### Changed
- `parse_line()` now rejects trailing whitespace. Previously it was
  silently ignored; scripts relying on that will need updating.

6Worked example: a review, in full

Here is a patch as submitted. Review it before reading on.

--- a/src/config.c
+++ b/src/config.c
@@ -40,6 +40,38 @@ static Config *global_config = NULL;
+/* Load a configuration file and install it. */
+int config_reload(const char *path)
+{
+    FILE *f = fopen(path, "r");
+    if (!f) return -1;
+
+    char *buf = malloc(8192);
+    fread(buf, 1, 8192, f);
+
+    free(global_config);
+    global_config = malloc(sizeof(Config));
+
+    char *line = strtok(buf, "\n");
+    while (line) {
+        char key[32], value[64];
+        sscanf(line, "%s = %s", key, value);
+
+        if (strcmp(key, "timeout")) {
+            global_config->timeout = atoi(value);
+        } else if (strcmp(key, "name") == 0) {
+            strcpy(global_config->name, value);
+        }
+        line = strtok(NULL, "\n");
+    }
+
+    fclose(f);
+    return 0;
+}

The review

blocking — line 45: unchecked allocation. malloc(8192) is not checked, and fread writes to buf immediately. On failure this is a null dereference. Same on line 50 for the Config.

blocking — line 46: buf is not null-terminated. fread returns a count that is ignored, and nothing writes a '\0'. strtok then reads past the end of the allocation. Capture the return value and terminate:

size_t n = fread(buf, 1, 8191, f);
buf[n] = '\0';

blocking — line 49: the old config is freed before the new one is valid. This is the defect the change was meant to avoid. If parsing fails, global_config points at freed memory and the next request dereferences it. Build the new config in a local, then swap:

Config *fresh = calloc(1, sizeof *fresh);
/* … parse into fresh, returning on error … */
Config *old = global_config;
global_config = fresh;
free(old);

blocking — line 55: strcmp used backwards. if (strcmp(key, "timeout")) is true when the strings differ. Every non-timeout key sets the timeout. Week 14's trap, and the compiler cannot help.

blocking — line 54: unbounded sscanf. %s into char key[32] with no width is a stack buffer overflow from a config file. Use %31s and %63s, and check the return value — a malformed line currently leaves both buffers uninitialized and they are used anyway.

blocking — line 58: strcpy into a fixed buffer. Unbounded copy of attacker-controlled text. snprintf(global_config->name, sizeof global_config->name, "%s", value).

blocking — buf is leaked. Allocated on line 45, never freed on any path.

blocking — f is leaked on the early return at line 43? No — the return precedes the fopen succeeding. But if the malloc check is added as suggested, that new path must close f. This is exactly why week 27 prefers one cleanup label over scattered returns.

question — is this called from more than one thread? strtok keeps hidden static state and global_config is swapped without synchronization. If reloads can be concurrent with requests, both are races. strtok_r at minimum.

suggestion — atoi cannot report failure. A malformed timeout becomes 0 silently. strtol with endptr, as in parse_port() two files over.

nit — the comment says what the function does; the interesting part is the ownership. Worth stating that the caller does not own the result and that the previous config is freed.

The revised version

/* Load a configuration file and install it as the active config.
 * On failure the previous configuration remains installed and is
 * unchanged. The caller does not own the result.
 * Returns 0 on success, -1 on failure with errno set where possible. */
int config_reload(const char *path)
{
    int      status = -1;
    FILE    *f      = NULL;
    char    *buf    = NULL;
    Config  *fresh  = NULL;

    f = fopen(path, "r");
    if (f == NULL) {
        goto cleanup;
    }

    buf = malloc(CONFIG_MAX + 1);
    if (buf == NULL) {
        goto cleanup;
    }

    size_t n = fread(buf, 1, CONFIG_MAX, f);
    if (ferror(f)) {
        goto cleanup;
    }
    buf[n] = '\0';                       /* fread does not terminate */

    fresh = calloc(1, sizeof *fresh);    /* calloc: every field defined */
    if (fresh == NULL) {
        goto cleanup;
    }

    char *save = NULL;                   /* strtok_r: no hidden state */
    for (char *line = strtok_r(buf, "\n", &save);
         line != NULL;
         line = strtok_r(NULL, "\n", &save)) {

        char key[32], value[64];
        if (sscanf(line, "%31s = %63s", key, value) != 2) {
            continue;                    /* skip malformed lines */
        }

        if (strcmp(key, "timeout") == 0) {
            long v;
            if (!parse_long(value, &v) || v < 0) {
                goto cleanup;            /* refuse rather than default to 0 */
            }
            fresh->timeout = (int)v;
        } else if (strcmp(key, "name") == 0) {
            snprintf(fresh->name, sizeof fresh->name, "%s", value);
        }
    }

    /* Only now is the old configuration replaced. */
    Config *old = global_config;
    global_config = fresh;
    fresh = NULL;                        /* ownership transferred */
    free(old);
    status = 0;

cleanup:
    free(fresh);                         /* NULL unless we failed */
    free(buf);
    if (f != NULL) {
        fclose(f);
    }
    return status;
}

What changed, and which week it came from

Eight blocking findings in twenty-eight lines, and every one is a topic from earlier in this course. That is what makes a checklist worth having: the defects are not exotic, and a reviewer working from memory will miss two or three of them on a Friday afternoon.

Practise it

Find an open pull request in a C project on GitHub and review it against section 3's list. Write the comments out, even if you do not post them, and then read what the actual reviewers said. The gap between your list and theirs is the most direct feedback available on how you read C.

7Common mistakes

9Where this leads

Week 45 turns the module you have been reviewing into something other people can install: versioning, soname, symbol visibility, pkg-config, and the ABI rules that decide whether adding a struct member is a patch or a major release.