Skip to content

Fix out-of-bounds write in isotopic proton removal#1

Open
tc-agent wants to merge 2 commits into
devfrom
fix-oob-write-isotope-proton
Open

Fix out-of-bounds write in isotopic proton removal#1
tc-agent wants to merge 2 commits into
devfrom
fix-oob-write-isotope-proton

Conversation

@tc-agent
Copy link
Copy Markdown
Owner

@tc-agent tc-agent commented Apr 9, 2026

Summary

Fix a heap-corrupting out-of-bounds array write in ichi_bns.c that causes crashes when processing InChI strings with large isotope mass differences (e.g. InChI=1/H/q+1/i1+222).

Root Cause

The bounds check for iso_atw_diff at ichi_bns.c:5973 (inside the FIX_IMPOSSIBLE_H_ISOTOPE_BUG guard) only validates the upper bound (<= NUM_H_ISOTOPES) but not the lower bound (> 0).

When iso_atw_diff is negative — which happens when a large isotope mass difference (e.g. +222) is truncated from short to char at ichirvr2.c:207, wrapping 223 to -33 — the array access nNumRemovedProtonsIsotopic[iso_atw_diff - 1] writes at index -34, far before the array start. This corrupts heap metadata, causing free(): invalid pointer when t_group_info->t_group is later freed in Create_INChI (ichimake.c:4554).

Fix

Add the missing > 0 lower bound check, consistent with all other similar checks in the codebase (ichirvr1.c:5552, ichirvr1.c:5653, strutil.c:3917).

Found by OSS-Fuzz. The coverage build has been failing since 2026-03-30 due to this crash during corpus merge.

@tc-agent
Copy link
Copy Markdown
Owner Author

tc-agent commented Apr 9, 2026

[CLAUDE CODE REVIEW - claude-opus-4-6]

Here's my review:


PR Review: Fix out-of-bounds write in isotopic proton removal

Correctness: PASS

The fix is correct. iso_atw_diff is a S_CHAR (signed char, line 158 of inpdef.h) that can be negative per its documentation (line 117: <0 => (mass) - (mass of the most abundant...)). The outer guard at line 5970 (if (at[0].iso_atw_diff)) only filters zero, not negative values. Without > 0, a negative iso_atw_diff passes the <= NUM_H_ISOTOPES check and causes nNumRemovedProtonsIsotopic[negative_index] — a clear OOB write.

The > 0 lower bound is consistent with how the codebase handles this field elsewhere (e.g., strutil.c:3917 uses 0 < new_at[n].iso_atw_diff).

Checklist Review

Item Status Notes
Diff minimality OK Single-line change, minimal
Correct repo OK Upstream source fix, not oss-fuzz config
Project conventions OK Matches existing style (> 0 && pattern used in strutil.c)
No formatting/whitespace changes OK
Coverage reporting N/A This is a bug fix, not a harness change
PR description OK Concise, explains the bug and fix
No checklist leak OK
No AI disclaimers OK

Minor observations

  • The #else branch at line 5979 still has the unchecked access. This is guarded by FIX_CURE53_ISSUE_NULL_DEREFERENCE_MAKE_A_COPY_OF_T_GROUP_INFO / FIX_IMPOSSIBLE_H_ISOTOPE_BUG being disabled. It's pre-existing and out of scope for this fix (those macros appear to be enabled in production builds), but worth noting.

Related PRs

None of the listed oss-fuzz PRs are directly related — they deal with build config, not source-level fixes. The existing #39064660 comment in the code references a prior OSS-Fuzz issue that led to the original bounds check (without the > 0).

LGTM

[CLAUDE CODE REVIEW - claude-opus-4-6]

claude and others added 2 commits April 24, 2026 18:39
The bounds check for iso_atw_diff in ichi_bns.c only validates the
upper bound (<= NUM_H_ISOTOPES) but not the lower bound (> 0). When
iso_atw_diff is negative (e.g. due to char truncation of large isotope
mass differences at ichirvr2.c:207), the array access
nNumRemovedProtonsIsotopic[iso_atw_diff - 1] writes before the array,
corrupting heap metadata and causing a crash in a later free().

Add the missing lower bound check, consistent with all other similar
checks in the codebase (ichirvr1.c:5552, ichirvr1.c:5653,
strutil.c:3917).

Found by OSS-Fuzz (coverage build crash on InChI=1/H/q+1/i1+222).
@tc-agent tc-agent force-pushed the fix-oob-write-isotope-proton branch from 500f01e to cd24ade Compare April 24, 2026 18:40
@tc-agent
Copy link
Copy Markdown
Owner Author

[CLAUDE CODE REVIEW - claude-opus-4-6]

LGTM (seeded for batch CI validation of inchi)

[CLAUDE CODE REVIEW - claude-opus-4-6]

@github-actions
Copy link
Copy Markdown

Fuzzing Coverage Report

Tested: project inchi · base 94c8718 → head cd24ade · 300s total fuzz budget · updated 2026-04-24 18:55 UTC · workflow run

Metric Before After Delta
Line coverage 14.6% (15377/105278) 14.0% (14692/105278) -0.7 pp
Branch coverage 9.2% (6132/66907) 8.6% (5756/66909) -0.6 pp
Function coverage 33.2% (398/1198) 32.9% (394/1198) -0.3 pp

Per-harness

Harness Lines before Lines after Δ
inchi_input_fuzzer 14.6% (15377/105278) 14.0% (14692/105278) -0.7 pp

Same harness config applied to both sides (baseline = base source + PR harness). Per-harness data from report_target/&lt;fuzzer&gt;/linux/summary.json. Full HTML reports in the workflow artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants