sbmlteam / libsbml

LibSBML is a native library for reading, writing and manipulating files and data streams containing the Systems Biology Markup Language (SBML). It offers language bindings for C, C++, C#, Java, JavaScript, MATLAB, Perl, PHP, Python, R and Ruby.
https://sbml.org/software/libsbml
Other
38 stars 28 forks source link

Segmentation fault when doing function expansion and unit inference after conversion to L3V2 #334

Closed avandecreme closed 10 months ago

avandecreme commented 1 year ago

The following program segfault when fed the following uncompressed file

#include <stdio.h>
#include <sbml/SBMLTypes.h>
#include "util.h"

int main (int argc, char *argv[])
{
  if (argc != 2)
  {
    printf("Usage: readSBML filename\n");
    return 2;
  }

  const char * filename = argv[1];
  SBMLDocument_t *d = readSBML(filename);

  int success = SBMLDocument_setLevelAndVersion(d, 3, 2);
  printf("Success: %d\n", success);

  ConversionProperties_t* props;

  props = ConversionProperties_create();
  ConversionOption_t * expandFuncDefOpt = ConversionOption_create("expandFunctionDefinitions");
  ConversionProperties_addOption(props, expandFuncDefOpt);
  SBMLDocument_convert(d, props);
  ConversionOption_free(expandFuncDefOpt);
  ConversionProperties_free(props);

  props = ConversionProperties_create();
  ConversionOption_t * inferUnitsOpt = ConversionOption_create("inferUnits");
  ConversionProperties_addOption(props, inferUnitsOpt);
  SBMLDocument_convert(d, props);
  ConversionOption_free(inferUnitsOpt);
  ConversionProperties_free(props);

  SBMLDocument_free(d);
}

From what I gathered, it is because tempUD1 is set to NULL here: https://github.com/sbmlteam/libsbml/blob/49ce5b40ead6862c1a610a569d0606f03a4fb4fa/src/sbml/units/UnitFormulaFormatter.cpp#L2635 The reason for that is that inverseFunctionOnUnits calls UnitDefinition::divide on two UnitDefinitions having different level and version (L2V4 like the original doc and L3V2 like the converted one), so we are falling in the following branch: https://github.com/sbmlteam/libsbml/blob/49ce5b40ead6862c1a610a569d0606f03a4fb4fa/src/sbml/UnitDefinition.cpp#L1467

I found two ways to avoid the crash:

  1. move the level/version conversion after function expansion and unit inference
  2. keep level/version conversion first but group function expansion and unit inference into one convert call like this:
    props = ConversionProperties_create();
    ConversionOption_t * expandFuncDefOpt = ConversionOption_create("expandFunctionDefinitions");
    ConversionProperties_addOption(props, expandFuncDefOpt);
    ConversionOption_t * inferUnitsOpt = ConversionOption_create("inferUnits");
    ConversionProperties_addOption(props, inferUnitsOpt);
    SBMLDocument_convert(d, props);
    ConversionOption_free(expandFuncDefOpt);
    ConversionOption_free(inferUnitsOpt);
    ConversionProperties_free(props);

Having level/version conversion in between function expansion and unit inference also segfault.

skeating commented 1 year ago

Thanks very much for this. As far as I know you are the first person to have used the UnitInference since I wrote it some years ago and there may well be undiscovered issues. But thank you for the detailed report this is very helpful