A few years ago, I was leading a team working on a custom embedded filesystem. One of our developers submitted a pull request for a new caching mechanism. It was elegant code—mathematically sound and it passed every single unit test we threw at it. I skimmed it, saw the logic was clean, and gave it a thumbs-up. Two weeks later, the system started rebooting spontaneously in the field. It turned out there was a tiny memory leak: a single 32-byte allocation that didn't get freed only when a specific, rare disk-write error occurred. Because we were reviewing for "feature correctness" and not "C correctness," we missed the one line of code that would eventually starve the kernel of memory.

That's the trap of C code reviews. In higher-level languages, you spend your time debating architecture or naming conventions. In C, you have to be a detective. You aren't just looking at what the code does; you're looking for what it might do when the heap is full, the pointer is null, or the input is exactly one byte longer than expected.

Hunting for the "Invisible" C Bugs

When you open a diff, your first instinct should be to ignore the "happy path." I assume the logic works for a moment and instead go hunting for the edges. The most common culprits in C are memory lifecycles and pointer validity. I always check the error paths first. If a function allocates memory at the top and has five different return -1; statements throughout the body, I immediately look for the corresponding free() calls before each return. It is incredibly easy to forget a cleanup call when you're rushing to handle an edge case.

Then, I look at the bounds. If I see a while loop or a for loop incrementing a pointer, I ask: "Who owns this memory, and how do we know we haven't stepped past the end of the buffer?" C won't stop you from reading the 101st element of a 100-element array; it'll just give you whatever garbage is sitting in RAM, or it'll segfault. I look for strncpy instead of strcpy, and I double-check that the null terminator is actually being placed where it belongs. It's tedious, but it's the only way to sleep soundly at night.

Balancing Pedantry with Progress

There is a fine line between being a rigorous reviewer and being the "nitpick" person that everyone dreads. I've been both. The key is distinguishing between correctness and style. If a developer used a while loop where a for loop would be slightly cleaner, I might mention it as a suggestion, but I won't block the merge. However, if they're casting the return value of malloc, I'll point it out—not because it's "wrong" in every compiler, but because it's redundant in C and can occasionally hide a missing header include.

I try to phrase my feedback as questions rather than commands. Instead of saying "You forgot to check for NULL here," I'll ask, "What happens to the system if malloc fails here?" This shifts the conversation from a critique of the person to a shared investigation of the code's robustness. It encourages the author to think like a reviewer, which eventually means they'll start catching their own mistakes before the PR even reaches me.

Leveraging Tooling Before the Human Touch

My biggest pet peeve is spending twenty minutes of a human's time finding a bug that a static analyzer could have found in two seconds. Before I even look at a C project's code, I want to see that it's been run through a set of automated guards. I expect to see that the code compiles with -Wall -Wextra -Werror. If the compiler is screaming warnings, the review shouldn't even start.

Beyond the compiler, I push for AddressSanitizer (ASan) or Valgrind reports. If a developer can tell me, "I ran this through ASan with 100% branch coverage and there are no leaks," I can stop worrying about the basic memory bookkeeping and spend my mental energy on the actual logic and concurrency issues. Use the tools to handle the mechanical checks so the humans can handle the conceptual ones.

📋 Practical Task

Auditing a Leaky String Concatenator

Below is a snippet of a C function designed to join two strings with a delimiter. This code "works" (it compiles and produces the correct output for standard strings), but it contains several critical C-specific flaws that would fail a professional code review.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char* join_strings(const char* s1, const char* s2, const char* delim) {
    int len = strlen(s1) + strlen(s2) + strlen(delim);
    char* result = (char*)malloc(len); 
    
    if (result == NULL) {
        return NULL;
    }

    strcpy(result, s1);
    strcat(result, delim);
    strcat(result, s2);

    return result;
}

int main() {
    char* combined = join_strings("Hello", "World", " ");
    printf("Combined: %s\n", combined);
    // The developer forgot something here!
    return 0;
}

Your Task: Perform a code review on this snippet. Identify at least three specific issues (ranging from memory bugs to undefined behavior) and explain why they are dangerous. For each issue, provide the corrected line of code or the missing logic required to make this production-ready.