Repository navigation
Read polymer Sgroups from V3000 molfiles - #293
Open
cm-beilstein wants to merge 7 commits into
Open
cm-beilstein wants to merge 7 commits into
cm-beilstein wants to merge 7 commits into
Conversation
Reproduces GH #146: the V2000 polyethyleneoxide molfile gives a polymer InChI with -Polymers, the equivalent V3000 molfile fails with error 101 because the V3000 reader drops star atoms and skips the SGROUP block. The V3000 test fails until V3000 polymer reading is implemented. Refs #146 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With -Polymers, V2000 polymer molfiles give a polymer InChI, but the equivalent V3000 file failed with error 101: the V3000 reader always dropped star atoms and skipped the whole SGROUP block. Keep star atoms when polymers are treated (they are the SRU end groups, as in V2000) and parse SRU/MON/COP/MOD/CRO/MER Sgroup lines (ATOMS, XBONDS, BRKXYZ, CONNECT, SUBTYPE, LABEL) into the same sgroups the V2000 M STY/SAL/SBL/... lines fill. Other Sgroup types are skipped; a malformed polymer Sgroup is reported as an error. Without -Polymers nothing changes. Fixes #146 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A V3000 molfile that ends inside the SGROUP block (no END SGROUP) makes MolfileV3000ReadSGroup loop forever. The test feeds such a file with and without -Polymers and expects an error return; it runs the call in a thread and exits with failure after a timeout so a hang fails the test instead of blocking ctest. Refs #146 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MolfileV3000ReadSGroup looped forever when the input ended (or a non-V30 line came) before END SGROUP. Leave the loop when no line can be read, so the existing "No V3000 SGroup end marker" error is reported. Ending there exposed a read past the input buffer: the API sets the string stream length to strlen + 1, so inchi_ios_str_getc consumed the terminating NUL and later inchi_sgets started beyond it. Make getc stop at the NUL. Refs #146 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Unit Test Coverage ReportCoverage Regression Summary
Find details on the base coverage at https://iupac-inchi.github.io/InChI/coverage/index.html Find details on this PR's coverage by downloading coverage-reports-40313986464 and opening html/index.html |
A 79-char LABEL fills MOL_FMT_SGROUP.smt[80] exactly; read_upto_delim then writes one NUL past it into alist.item, which ASan reports as a heap-buffer-overflow when the atom list is read. A trailing BRKXYZ keyword without value makes V3000ReadBracket compare an uninitialised buffer; ASan cannot see that, so this test documents the expected error return only. Refs #146 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
read_upto_delim accepts a token of maxlen - 1 chars, copies it with mystrncpy(field, p, n + 1), which already NUL-terminates at field[n], and then wrote field[n + 1] = '\0' one byte past the buffer. A 79-char V3000 Sgroup LABEL thus overwrote alist.item after smt[80]. Drop the redundant write. All other callers (V3000 field, keyword, list and haptic readers in mol_fmt3.c) only read field as a string ending at field[n], so they are unaffected. Refs #146 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With a trailing BRKXYZ keyword and no value the line pointer is NULL, read_upto_delim returns -1 without writing field, and strcmp then read the uninitialised buffer. Fail on the read error first. Refs #146 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #146.
With
-Polymers, a V3000 molfile gave Error 101 where the equivalent V2000 one gave a polymer InChI. Two causes inmol_fmt3.c:*atoms, so bonds pointed at missing atoms. They are now kept when polymers are on.M S**lines fill. Other Sgroup types are skipped.Without
-Polymersbehaviour is unchanged.Also fixed:
MolfileV3000ReadSGrouplooped forever on a file truncated beforeEND SGROUP.inchi_ios_str_getc(ichi_io.c) read past the terminating NUL, causing a buffer over-read in the nextinchi_sgetson such input.Tests (
test_github_146): V2000 and V3000 polyethyleneoxide give the same InChI; truncated SGROUP block with and without-Polymersreturns an error instead of hanging.Known limits / open:
*atom as centre; with-Polymersthat star is now kept. Untested combination, left as is.-Polymers, V3000 star atoms are still dropped while V2000 gives a Zz InChI.