DEV Community

R Sai pranav
R Sai pranav

Posted on

Two stack overflows hiding in plain sight

#c #opensource #security #debugging

Most memory bugs I've fixed in open-source C weren't clever. They were a buffer sized for the "usual" input, sitting next to code that never promised the usual input. Here are two that got merged, and what each one taught me.

Bug 1: pgagroal, strcat into 512 bytes

pgagroal is a connection pooler for PostgreSQL. Its CLI has a list_limits command that prints a database=alias,alias,... column. It built that string like this:

char db_alias_string[DB_ALIAS_STRING_LENGTH];   /* 512 */
pgagroal_snprintf(db_alias_string, sizeof(db_alias_string), "%s", database);
...
strcat(db_alias_string, "=");
...
strcat(db_alias_string, ",");
strcat(db_alias_string, alias);
Enter fullscreen mode Exit fullscreen mode

The first line is bounded. Everything after it isn't: strcat does no bounds checking, and the loop runs once per alias.

So the real question is not "is strcat dangerous" — everyone knows that. It's how big can this string legally get? The answer is in pgagroal.h, not in cli.c:

MAX_DATABASE_LENGTH = 256, MAX_ALIASES = 8

worst case the config allows : 2304 bytes  -> overflows 512 by 1792
Enter fullscreen mode Exit fullscreen mode

The worst case is easy to dismiss as pathological. What made it worth fixing is that you don't need it:

30-char database + 8 aliases of 60 characters = 519 bytes -> overflows
Enter fullscreen mode Exit fullscreen mode

That's an ordinary configuration.

The fix I didn't make:bump 512 to 2304. It works today and breaks the moment someone raises MAX_ALIASES. The buffer's size was an assumption about another file's constants, and assumptions like that drift.

The fix that got merged (#1036): use the project's existing pgagroal_append() / pgagroal_append_char() helpers, which grow the allocation as they go. There's no fixed buffer left to overrun, and the DB_ALIAS_STRING_LENGTH constant disappears with it. This was also the direction the maintainer had already asked for in that area, which mattered more than my preference.

One honest caveat I put in the PR: I didn't trigger the overflow. The test suite needs a live PostgreSQL, so the evidence is the arithmetic from the configuration limits. Saying that plainly was better than implying a test I hadn't run.

Bug 2: GRASS GIS, "%.8f" into 30 bytes

GRASS GIS formats map-region coordinates with a helper in lib/gis/wind_format.c:

static void format_double(double value, char *buf, int full_prec)
{
    if (full_prec)
        sprintf(buf, "%.15g", value);
    else
        sprintf(buf, "%.8f", value);
}
Enter fullscreen mode Exit fullscreen mode

%.8f looks bounded because of the .8. It isn't. The precision limits digits after the decimal point; the digits before it are unbounded. For DBL_MAX the result is 318 characters.

Meanwhile four callers passed buffers sized for a normal coordinate:

raster/r.coin/print_hdr.c   char north[30], south[30], east[30], west[30];
raster/r.report/header.c    char north[50], ...
ps/ps.map/map_info.c        char east[50], ...
display/d.where/where.c     char buf1[50], buf2[50];
Enter fullscreen mode Exit fullscreen mode

Can a user actually get a huge value there? Yes: g.region n=1e20 in a non-lat/lon location is parsed by a bare sscanf with no magnitude check, and the region's north then flows into those buffers when any of the four programs prints its header.

Reproducing it without a multi-GB build.I copied format_double() verbatim into a small harness and compiled it with -fsanitize=address:

char buf[30];                  /* same as r.coin */
format_double(1e20, buf, 0);

==10937==ERROR: AddressSanitizer: stack-buffer-overflow
WRITE of size 31
Enter fullscreen mode Exit fullscreen mode

The boundary was exact: 9e19 produces 29 characters and is clean; 1e20 produces 30, one more than 30 bytes can hold with the terminator, and overflows.

Picking the fix (#7943). The "right" fix is arguably a bounded snprintf inside format_double(). But it's called from many places with no size parameter, so that means changing an API across the tree — a bigger decision than the bug. I sized the four buffers to 320 bytes (sign + 318 digits + terminator), commented why, and offered the API change as a follow-up if the maintainers preferred it. They merged the small fix.

What both bugs had in common

1.The bound lived somewhere else.In pgagroal it was a header's configuration limits; in GRASS it was what sscanf would accept. Reading the buffer's line tells you nothing. Follow the data back to where its size is actually decided.
2."Reachable from an ordinary input" is the bar. A worst case gets waved away; 519 bytes from a normal config, or n=1e20 from a normal command, doesn't.
3.Match the fix to the decision you're allowed to make. Removing the buffer (pgagroal) and resizing it (GRASS) were both right, because of what each project's maintainers wanted and how far each change reached.
4.Say exactly what you verified. "Proven from limits, not triggered" and "reproduced in an ASan harness, not the full build" made both PRs easier to trust, not harder.

These two are part of 50+ patches I've had merged across pgmoneta, pgagroal, pgvictoria, GRASS GIS, JabRef, GNU Radio and the Kubernetes docs. More at saipranav.me and github.com/Pranav-error.

Top comments (0)