search

vincefn / objcryst

Free Objects for Crystallography : Fox / ObjCryst++
Other
23 stars 10 forks source link

Fix spacegroup recursion #42

Closed pavoljuhas closed 5 years ago

pavoljuhas commented 5 years ago

Fix segfault due to infinite recursion from SpaceGroup("invalid"). Throw ObjCrystException for invalid SG name in constructor and ChangeSpaceGroup.

I am not sure we need the second try-catch guard, because at that point the mpCCTbxSpaceGroup exists. Perhaps that try-catch guard can be removed

I am attaching two example source files to test the behavior with invalid SG names.

tsg1.cpp.txt - test constructor tsg2.cpp.txt - test ChangeSpaceGroup

vincefn commented 5 years ago

Ok, I'll trust your changes. See that indeed now checking for a null pointer (mpCCTbxSpaceGroup!=0) before deleting is unnecessary.

pavoljuhas commented 5 years ago

@vincefn - Sorry I found an error. Could you please also merge a38c06e0efe8eda306e201decb295062d8748b7f, i.e.,

git pull https://github.com/pavoljuhas/objcryst.git fix-spacegroup-recursion

I found the changes broke reading of CIF files, because CreateCrystalFromCIF checks several space group origins, which may now throw exception. I have added exception handling in a38c06e0efe8eda306e201decb295062d8748b7f and the pyobjcryst tests are passing now - see https://github.com/diffpy/pyobjcryst/pull/22

Let me know if I should open a new PR.

vincefn commented 5 years ago

It will be easier if you submit a PR, yes- Thanks

pavoljuhas commented 5 years ago

Sure. I have noticed one more thing - ObjCrystException writes out the ObjCryst++ state to an xml file, but now this happens within a successful CreateCrystalFromCIF call. I can think of several ways how to fix that:

  1. change ObjCrystException to write XML only when build with the __DEBUG__ macro
  2. temporarily set ObjCrystException::verbose = false in CIF reader to suppresses XML
  3. throw some other exception from SpaceGroup("bad") say invalid_argument which would avoid the XML issue

What would be your preference? I suppose 3 might be the easiest.

vincefn commented 5 years ago

3 sounds better- I know writing the xml when an exception is caught is quite ugly as too many things can go wrong, it's mostly a desperate recovery attempt when soothing goes wrong within Fox...