Why Every Bug Fix Needs Three Steps

Fixing a security vulnerability is rarely just about patching the immediate defect. From my work hardening the Exiv2 metadata library, I've settled on a three-step discipline for every bug fix: add a regression test first, then fix the bug, and finally hunt down and fix variants of the same class of error. A representative pull request for GHSA-pvjp-m4f6-q984 shows all three steps as separate commits.

The middle step needs no justification. The other two deserve explanation, particularly why the regression test belongs before the fix.

Regression Tests: More Than Just Proof

Adding a regression test before the fix lets you demonstrate that the test fails on the vulnerable code and passes once the patch is applied. Beyond that simple verification, regression tests serve two less obvious purposes.

  • Coverage for defensive conditions. Bug fixes frequently introduce new conditional checks. A regression test ensures those conditions are actually exercised. If a future developer removes what looks like an unnecessary guard, the test will fail and reveal why it was there.
  • Better fuzzing corpora. Fuzzers perform best with a corpus of meaningful inputs. For Exiv2, regression test files live in test/data and double as fuzzing corpus material. Every added test therefore improves the fuzzing setup as well.

Skipping regression tests is tempting, especially under release pressure. The reality, though, is that if you fix a subtle bug once, you don't want to repeat the diagnosis later.

Look for Nearby Bugs

There is a school of thought that says fix only what is verifiably broken and nothing else, particularly when patching a released version. The fear is that touching unrelated code introduces new problems. That concern is misdirected. Security history shows repeatedly that one bug almost always has cousins nearby. Vendors that patch only the exact reported defect, sometimes even only its symptoms, leave those relatives in place—to the frustration of security researchers.

Treat every bug as an opportunity to add a few extra defensive fixes.

Example: Integer Division by Zero

The crash in GHSA-pvjp-m4f6-q984 (CVE-2021-34335) comes from an integer divide by zero in minoltamn_int.cpp:

long    focalLength = getKeyLong  ("Exif.Photo.FocalLength"      ,metadata);
long    focalL35mm  = getKeyLong  ("Exif.Photo.FocalLengthIn35mmFilm",metadata);
long    focalRatio  = (focalL35mm*100)/focalLength;  <===== Divide by zero

On line 2174, focalLength is read from image metadata, so an attacker controls its value. A zero value divides on line 2176 and crashes the process.

The fix addressed the identical pattern on line 2183, but I also searched for integer divisions that could crash via the less obvious INT_MIN/-1 case. The CodeQL query:

/**
 * @kind: problem
 */
import cpp
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis

from DivExpr div, Expr rhs
where
  rhs = div.getRightOperand() and
  div.getType() instanceof IntegralType and
  not lowerBound(rhs) > 0
select rhs, "Possible integer divide by zero. Type: " + rhs.getType().toString()

The query locates integer divisions where the right operand is not provably greater than zero. Exiv2 never intentionally divides by a negative number, so any suspicious right-hand operand is worth inspecting; a negative divider could enable the INT_MIN/-1 crash. The SimpleRangeAnalysis library computes range bounds, but only handles relatively simple cases, so unknown values default to the full type range. That makes the query noisy. Running it against current Exiv2 yields 24 results, all of which I believe are false positives. For example, this division in value.hpp is clearly safe, but the surrounding logic is too complex for range analysis to prove:

ok_ = (value_.at(n).second > 0 && value_.at(n).first < LARGE_INT);
if (!ok_) return 0;
return value_.at(n).first / value_.at(n).second;

There is always a trade-off between refining the query and manually reviewing results. Since integer divisions are uncommon in this codebase, manual auditing was faster in this case. Other query types are worth investing in more heavily, as I'll discuss in a later post.

Example: Prefer at() Over operator[]

std::vector offers two indexing methods. The at() method throws on an out-of-bounds index; operator[] does not. Unless bounds are certain, at() is the safer choice.

An out-of-bounds access in value.hpp caused the crash in #1706:

template
long ValueType::toLong(long n) const
{
    ok_ = true;
    return static_cast(value_[n]);
}

toLong() is a utility called from many places; any single call site could pass an out-of-bounds index. Auditing every caller would be substantial work, so switching this function to at() removes a whole class of failure at once. The pull request replaced several more instances of operator[] in the same header, using this query:

/**
 * @kind: problem
 */
import cpp
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis

from FunctionCall call, ClassTemplateInstantiation t, TemplateClass c
where
  call.getQualifier().(VariableAccess).getTarget().getName() = "value_" and
  call.getTarget().getName() = "operator[]" and
  t = call.getQualifier().getType().getUnspecifiedType() and
  c = t.getTemplate() and
  c.getSimpleName() != "map"
select call, "Unsafe use of " + c.getSimpleName() + "::operator[]."

The restriction to vectors named value_ keeps the query simple—without it, the search returns 356 results, too many for a quick variant hunt. The simplicity did cost something: a later crash from the same operator[] pattern appeared in GHSA-v5g7-46xf-h728, indicating the query had missed variants. That prompted a more rigorous version of the query, discussed in a future post.

Further Query Examples

Several other Exiv2 issues were addressed using quick, one-off CodeQL queries to find related problems:

  • Uninitialized local variable causing non-determinism: #1737
  • SIGSEGV from dereferencing an end iterator: #1758
  • Infinite loop from integer overflow in a loop counter: #1766

Security bugs are seldom isolated. If you are already doing a release to patch one vulnerability, investing the same effort to harden nearby code adds real value without much additional overhead. A simplistic CodeQL query that returns false positives is still a useful triage tool when you're looking for more bugs to fix; the problem only arises when you need a precise, enforceable rule. The progression here—from a quick variant search to a query that belongs in continuous integration—is the practical path to making safety improvements stick.