Skip to content

Signed-overflow UB in db.c block-size computation (ctbytes - variance) with on-disk operands #898

Open

Description

@jsavin

Rationale

dbrefhandle and its sibling compute a block's payload length as a signed subtraction of two values read from the database file, then validate the result by testing for negative. Signed overflow is undefined behavior in C, so that post-hoc check is not a reliable guard: the compiler is entitled to assume it cannot happen and optimize the branch accordingly. Both operands come from on-disk block headers, which means a corrupted or hostile .root controls them.

Pre-existing; not introduced by any current work. Found while reading this path for the --diff-roots walker (PR #897), which calls dbrefhandle_context to read stored blocks.

Current State

Common/source/db.c:2128 (in dbrefhandle):

    if (!dbreadheader (a, &flfree, &ctbytes, &variance))
        return (false);

    ct = ctbytes - (long) variance;

    if (flfree || (ct < 0)) { /*probably a bad address*/

Common/source/db.c:2069 has the same shape:

	if (!dbreadheader_core(a, &flfree, &ctbytes, &variance, header_size, DB_FNUM_USE_GLOBAL))
		return (false);

	ct = ctbytes - (long) variance;

	if (flfree || (ct < 0)) {

Both ctbytes and variance are decoded from the block trailer/header on disk. A crafted or damaged database can therefore drive the subtraction out of range, and the ct < 0 test that is supposed to catch it is exactly the check UB permits the optimizer to discard.

Desired Outcome

  1. Perform the subtraction in a type where the overflow is defined and the range check is meaningful -- e.g. compute in uint64_t/int64_t and bounds-check before narrowing, or clamp both operands to sane maxima immediately after they are read.
  2. Validate ctbytes and variance against the file's actual size at the point they are decoded, so an impossible header is rejected where it is read rather than downstream.
  3. Apply the same treatment to any other ctbytes - variance sites (grep shows these two; worth a sweep for the pattern).
  4. Regression test: a fixture database whose block header carries extreme ctbytes/variance values must be rejected cleanly, not read.

Notes

Severity is bounded by the fact that opening an untrusted .root is already a high-trust operation, but this sits on the read path that every ODB consumer uses -- including read-only tools that are otherwise safe by construction. Worth fixing at the decode site so callers inherit the guarantee.

Metadata

Metadata

Assignees

No one assigned

    Labels

    priority/p2Medium priority - nice to havescope/quick-win< 4 hours, obvious solutiontype/bugDefect in existing functionality, needs fixworkstream/database-architectureDatabase format, migration, serialization

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions