IUPAC-InChI / InChI

Main InChI repository
MIT License
60 stars 7 forks source link

Crash in v1.07.1 source in InChI I/O testing #44

Closed baleland closed 1 month ago

baleland commented 1 month ago

ichiprt1.c line 3714 makes a call to str_AuxTautTrans() which fills/uses any non-null arguments,

    AT_NUMB           *nTrans_n,
    AT_NUMB           *nTrans_s,

and then FREES non-null pointers,

    if (nTrans_n)
    {
        inchi_free( nTrans_n );
    }
    if (nTrans_s)
    {
        inchi_free( nTrans_s );
    }

however in the caller, those are just DECLARED pointers, not allocated, which are then allocated by a call to bin_AuxTautTrans that allocates based on num_components. The crash in the inchi_free is highly suggestive of some type of memory overwrite.

CMD> inchi-1.exe BUG44_20240812.sdf.txt /FixedH /RecMet

( empty BUG44_20240812.sdf.txt.txt output (crash) )

BUG44_20240812.sdf.txt

djb-rwth commented 1 month ago

Hi again @baleland, Unlike bug #43, this crash only appears if GCC compiler is used on Ubuntu, but fails to reproduce on Windows if compiled with MSVC where everything seems to be working perfectly. I will mark it as bug anyway.

baleland commented 1 month ago

well, compiler differences are definitely challenging. I will note that the output I provided above is with the inchi-1.exe you provided from a prior bugreport on windows, but there have been code updates since then, clearly.

djb-rwth commented 1 month ago

Hi @baleland, I think I managed to fix the bug. Please be so kind as to swap ichiprt3.c with the version I am attaching (please remove .txt extension) and let me know if it works. Thanks again. ichiprt3.c.txt

baleland commented 1 month ago

I can confirm that this module fixes the crash for this case.

That allowed me to run the entire test to compare to the v1.06 standards. The test is reading SDF molecules and "round-tripping" via an InChi (& key) generated with /FixedH /RecMet. The result is 314 I/O failures, compared to 0 for v1.06.

djb-rwth commented 1 month ago

Hi @baleland, Thanks for the report. Again, this will remain as one of the standard tests for all future testing. Getting back to issue #43.