Validate At the Lowest Public Boundary

pygeohash has a C extension for its hot path, which I’ve written about a few times this year. The Python layer wraps it and validates arguments before calling in. That arrangement felt airtight to me, and it has a hole in it that’s worth generalizing well beyond this library.

The setup

The public Python function checks its inputs:

def encode(latitude, longitude, precision=12):
    if not 1 <= precision <= 12:
        raise ValueError("precision must be between 1 and 12")
    return cgeohash.encode(latitude, longitude, precision)

And it needs to, because the C function writes into a fixed-size stack buffer:

static PyObject *geohash_encode(PyObject *self, PyObject *args) {
    char geohash[13];
    ...
    while (hash_index < precision) {
        /* writes geohash[hash_index++] */
    }
}

Twelve characters plus a terminator. The Python check guarantees precision never exceeds 12, so the loop never runs off the end. Done, right?

The hole

The C extension is an importable module:

>>> from pygeohash.cgeohash.geohash_module import encode
>>> encode(0.0, 0.0, 50)

That call goes nowhere near the Python wrapper. The loop runs fifty times into a thirteen-byte stack buffer, which is a stack buffer overflow with all the undefined behavior that implies: corrupted locals, a smashed return address, a crash, or in the worst case something more interesting than a crash.

I never documented that import path. I never mentioned it in the README. It doesn’t appear in __all__. And none of that matters, because Python has no way to make a module private. If it’s importable, it’s public.

The fix belongs in C

The temptation is to make the module harder to reach: rename it with a leading underscore, hide it deeper in the package, document that it’s internal. All of that is a request rather than a boundary, and none of it helps when the caller is a curious user, a vendoring script, or something that found the symbol through introspection.

The check goes where the buffer is:

static PyObject *geohash_encode(PyObject *self, PyObject *args) {
    double latitude, longitude;
    int precision;
    char geohash[13];

    if (!PyArg_ParseTuple(args, "ddi", &latitude, &longitude, &precision))
        return NULL;

    if (precision < 1 || precision > 12) {
        PyErr_SetString(PyExc_ValueError,
                        "precision must be between 1 and 12");
        return NULL;
    }
    ...
}

Immediately after argument parsing, before any write. It’s five lines, it costs one comparison per call on a function that does far more work than that, and it means the C layer is self-protecting no matter who calls it.

Note that this is purely additive. Every valid input, precision 1 through 12, behaves exactly as before. The only behavior change is that inputs which were previously undefined behavior now raise a ValueError, which is a strictly better thing for them to do.

Test through the boundary you’re defending

The test has to go through the C module directly, because a test through the Python wrapper proves nothing about the C function:

def test_c_encode_validates_precision_directly():
    from pygeohash.cgeohash.geohash_module import encode, encode_strictly

    for fn in (encode, encode_strictly):
        for bad in (0, 13, 50, -1):
            with pytest.raises(ValueError):
                fn(0.0, 0.0, bad)
        # valid precisions still work through the C layer
        assert len(fn(42.6, -5.6, 5)) == 5
        assert len(fn(42.6, -5.6, 12)) == 12

That last pair matters as much as the raises. A validation check that’s slightly too aggressive silently breaks the legitimate top of the range, and nothing in the failing tests would point at it.

The sibling case: non-finite input

While in there, the same reasoning applies to a second class of input. What does the encoder do with float('nan')?

The function clamps latitude to the valid range and wraps longitude. Both operations are comparison-based, and every comparison against NaN is false, so NaN survives the clamp untouched and flows into the bit-packing arithmetic. You get output. It means nothing.

The ordering rule I now apply to any numeric entry point:

if (!isfinite(latitude) || !isfinite(longitude)) {
    PyErr_SetString(PyExc_ValueError, "coordinates must be finite");
    return NULL;
}
/* only now clamp and normalize */

Finite, then in-range, then normalize. Clamping without a finiteness check is worse than not clamping at all, because it gives you the appearance of having handled the input.

Three habits for a native extension

Validation belongs at the boundary that cannot be bypassed, and in Python that boundary is lower than most people’s mental model of their own package.

Three things I’d apply to any package with a native extension:

1. Treat every importable module as public API. Underscores and documentation are conventions, not access control. If a function can be reached, assume it will be.
2. Put memory-safety checks adjacent to the memory. A bounds check in a different language, in a different file, is a check that a refactor can remove without any test noticing.
3. Test the low-level entry point directly. If your only tests go through the wrapper, your wrapper is tested and your extension is not.

The wrapper’s validation is still worth having, because it produces better error messages earlier. It just isn’t the thing standing between a user and a stack write.