[XERCESC-2233] DFAContentModel::buildDFA(): fix memory leaks when OutOfMemoryException occurs

[rouault]

Fixes GDAL's https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=41335

[rleigh-codelibre]

How safe is the saving and re-throwing of the exception? Is there any potential for OutOfMemoryException& e to be a reference to a base class? Could e end up being truncated as a result? A direct throw to rethrow the existing exception might be safer overall, and avoid the need for a goto.

Could we achieve the same effect with a higher-level try/catch block within the function, and avoid the saving of the exception and the goto?

[rouault]

possibly. I didn't want to change the code too much. Your suggestion will probably involve moving the cleanup code in a lambda function that will be called at the end of the nominal code path and in the catch() block.

[rouault]

I've updated the commit with the above strategy I mentioned in #44 (comment). The diff might be hard to read because of the indentation changes, but the changes themselves are relatively simple and consist in:

• moving

  xerces-c/src/xercesc/validators/common/DFAContentModel.cpp

  Line 1006 in 8ac9637

  while (unmarkedState < curState)

  to

  xerces-c/src/xercesc/validators/common/DFAContentModel.cpp

  Line 1264 in 8ac9637

  } //while

  of one indentation level in a try {} block
• moving

  xerces-c/src/xercesc/validators/common/DFAContentModel.cpp

  Line 1266 in 8ac9637

  // Store the current state count in the trans table size

  to

  xerces-c/src/xercesc/validators/common/DFAContentModel.cpp

  Line 1329 in 8ac9637

  fMemoryManager->deallocate(leafSorter);

  in the finalizeProcessingAndCleanup() lambda
• calling that lambda after the try {} block in the nominal case, and in the catch ( OutOfMemoryException ) block
• and doing the memory allocations https://github.com/apache/xerces-c/blob/master/src/xercesc/validators/common/DFAContentModel.cpp#L1228 to

  xerces-c/src/xercesc/validators/common/DFAContentModel.cpp

  Line 1249 in 8ac9637

  }

  in a inner try {} catch (OutOfMemoryException) block, to do specific cleanups of those temporary arrays in case an exception is thrown

[scantor]

I saw mention of a lambda? I would have to imagine that's out of bounds for the branch given the need for older compiler support, though I may be wrong on that. This is a pretty huge patch to code I don't know so I'd be reluctant. Is the original smaller patch at least enough of an improvement to consider?

[rouault]

I believe the original patch was rouault@f7e7633 . I guess it is equivalent to the new one, but my memory may lie

[scantor]

Given that this is just fixing a memory leak in a case where the process is going to die anyway, I'm inclined to leave it out of the branch and this patch release. That's not a good enough reason to make a non-trivial change to code I don't know at all. Can always revisit later.