Skip to content

Fix six KET monomer crashes, hangs and lost errors found under Guard Malloc - #41

Open
gordlin wants to merge 1 commit into
masterfrom
gordon/ket-monomer-memory-safety
Open

gordlin wants to merge 1 commit into
masterfrom
gordon/ket-monomer-memory-safety

Conversation

@gordlin

@gordlin gordlin commented Oct 2, 2026 •

Copy link
Copy Markdown

Fixes the native crashes, hang and lost errors that main works around in
main/unc/materials/chemical/ket_structure_loading.py (uncountableinc/main#186765 and
uncountableinc/main#186767, MAT-90161 and MAT-90032), and bumps the fork to 1.46.0+unc36.
Bumping main to the new wheel, and removing the IMPROVE-marked monomer-id renumbering there, is a
separate PR.

How they were found

The 1,012 Ketcher autotest KET files that hold monomers, run through
loadKetDocument(...).expandMonomers() + molfile(), loadStructure and loadQueryMolecule
under macOS Guard Malloc (DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib), which turns an
out-of-range read into a fault.

The faults

All six are still on upstream master. Each site carries an [Uncountable] comment that says what
upstream does.

  1. mm_expand.cpp used monomer ids as indexes. getDimensions fills dimensions in
    monomersIds() order, but bfsPropagate and placeRingMonomers read dimensions[stoi(id)],
    and getGraph added the vertex stoi(id). Ketcher writes ids such as "635". The expansion
    now maps id to index, resolves endpoints by ref, skips bonds to molecule atoms and hydrogen
    bonds, seeds every ring-free component (an unseeded one failed in applyTransformations), and
    checks leaving-group atom indexes.
    • indigoExpandMonomers ended in INDIGO_END(0). No wrapper treats 0 as a failure, so every
      expansion error was lost, and the monomers were silently left without a layout. It now returns
      -1. This is why fault 1 often showed no crash: getGraph threw first, and nobody saw it.
  2. An ambiguous monomer was cast to KetMonomer, and the expansion wrote a transformation past
    the end of it. It also read the selected flag at the wrong index.
  3. {"moleculeId": "molN"} took N as the molecule's position. A document whose only molecule
    is mol1 read past mol_mappings. MoleculeJsonLoader now looks the ref up, checks the atom
    index, and accepts an integer atomId. The KetDocument saver renamed the molecule node to
    mol0 but kept mol1 in the connection, so it now renames the endpoint too.
  4. loadKetDocument hung on any rg-label with $refs. The loop over $refs in
    KetMolecule::parseKetAtoms incremented i instead of r.
  5. The KetDocument saver wrote a flip under the key "shift". With a rotate, the loader then
    read shift.x from a string. A flip-only transformation was dropped.
  6. KetObjWithProps::parseOptsFromKet called GetString() on Ketcher's integer atomId, and
    rapidjson's assertion aborted the process. Values are now type-checked, and a string property
    also takes an integer.

The JSON template parser also checks leaving-group atom indexes, which loadQueryMolecule read
unchecked.

Behaviour change

Because expansion errors were lost, documents with non-sequential ids, a bond to a molecule, or a
hydrogen bond were "expanded" without a layout: every monomer kept its drawn position and no
rotation. They now get the layout that sequential-id documents always got. Hydrogen bonds are left
out of that layout. Without that, the base pairs of Antisense-expected.ket close rings that put
atoms on top of each other.

Verification

Under Guard Malloc, on a native macOS build of this branch and of 6d6ae47c8:

expand crash / hang loadStructure crash loadQueryMolecule crash
6d6ae47c8 166 / 2 12 12
this branch 0 / 0 0 0

Of the 168 expand crashes and hangs, 98 now succeed and 70 raise an IndigoException from the
molfile saver: 59 "Ambiguous monomer cannot be saved to molfile" and 11 for an unknown or
unresolved monomer, which those test files contain on purpose. All 24 load crashes now succeed. The remaining expand errors were already errors on 6d6ae47c8.

Five files go from success to an error: Side-Chain-Connections/1.2, 1.3, 1.4 and 10,
and Sequence-Mode-Replacement/phosphate to sugar connected two sequences of presets (U). On
6d6ae47c8 their layout failed silently (array: invalid index 428), and renumbering the ids does
not change that, because upstream's getGraph read the digits of the ref. So they always
expanded with no layout. Now upstream's layout runs, and its ring placement puts side-chain loops on
top of each other: expandMonomers() and molfile() succeed, but loading that molfile again
raises "stereocenters: angle between bonds is too small" unless ignore-stereochemistry-errors is
set. A heuristic that skips rings sharing a placed monomer fixed four files and broke a fifth, so it
is not in this PR.

basic/unc_ket_monomer_expansion covers every fault. Against 6d6ae47c8 its layout checks print
False and it then aborts. Fault 2 does not crash without Guard Malloc, so that case documents the
fix rather than proves it. The hydrogen-bond case was checked against a build with only that skip
removed, and fails there.

Full integration suite on the native macOS build: 302 passed, 14 failed on this branch; 301 passed
and the same 14 failed on 6d6ae47c8 (run without the new test, which aborts there), each with
byte-identical output. The 14 are macOS float and font differences. CI on this PR is green.

🤖 Generated with Claude Code

…Malloc

Running the 1,012 Ketcher autotest KET files that hold monomers through
loadKetDocument(...).expandMonomers() and loadStructure under macOS Guard
Malloc found these faults in upstream 1.46. All six are still on upstream
master.

1. mm_expand.cpp indexed its per-monomer dimensions and graph vertices by
   monomer id, but Ketcher writes ids such as "635". It now maps each id to
   its position in monomersIds(), resolves endpoints by ref, skips bonds to
   molecule atoms and hydrogen bonds, seeds every ring-free component, and
   checks leaving-group atom indexes. indigoExpandMonomers ended in INDIGO_END(0), which no wrapper
   treats as a failure, so a document with non-sequential ids or a bond to a
   molecule was silently left without a layout; it now returns -1.
2. mm_expand.cpp cast an ambiguous monomer to KetMonomer and wrote a
   transformation past the end of it.
3. MoleculeJsonLoader took the N in "molN" as the molecule's position, so a
   document whose only molecule is "mol1" read past mol_mappings. It now
   looks the ref up and checks the atom index, and the KetDocument saver
   renames connection endpoints to the mol<i> names it gives the nodes.
4. KetMolecule::parseKetAtoms looped over "$refs" with i++ instead of r++,
   so any rg-label with $refs hung loadKetDocument.
5. KetDocumentJsonSaver wrote a monomer's flip under the key "shift", and the
   loader then read shift.x from a string; a flip-only transformation was
   dropped.
6. KetObjWithProps::parseOptsFromKet called GetString() on Ketcher's integer
   atomId, and rapidjson's assertion aborted the process. Values are now
   type-checked, and a string property also takes an integer.

The JSON template parser also checks leaving-group atom indexes, which a
query molecule read unchecked.

basic/unc_ket_monomer_expansion covers every case. Bumps to 1.46.0+unc36.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@gordlin
gordlin marked this pull request as ready for review October 2, 2026 19:38
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.

1 participant