Xerces-C++
  1. Xerces-C++
  2. XERCESC-1974

Memory leak occurs if an exception is thrown in TranscodeToStr or TranscodeFromStr constructors

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.1.1
    • Fix Version/s: 3.2.0
    • Component/s: Utilities
    • Labels:
    • Environment:
      Any

      Description

      The constructor for TranscodeToStr calls TranscodeToStr::transcode(). That method allocates memory for fString, but it is possible for an exception to occur shortly thereafter; it might be thrown directly by transcode(), or by the transcoder method call trans->transcodeTo(). Since we are still in the constructor, TranscodeToStr's destructor will not be called during the stack unwind to clean up (see http://www.parashift.com/c++-faq-lite/exceptions.html#faq-17.10 ). Therefore, we have to explicitly make sure fString will be deallocated. Since the current code doesn't do that, it leaks memory. TranscodeFromStr is in the same situation.

      I have a simple patch that I expect to post later tonight.

        Activity

        Hide
        Lee Doron added a comment -

        This patch is from the latest checked-in code in the development head of src/xercesc/util/TransService.cpp (revision 1138012).

        Show
        Lee Doron added a comment - This patch is from the latest checked-in code in the development head of src/xercesc/util/TransService.cpp (revision 1138012).
        Hide
        Lee Doron added a comment -

        By the way, two more notes. First, I noticed inconsistent usage of tabs and spaces for indentation in the transcode() methods. I didn't want to muddy up the patch with whitespace changes, but you might want to clean that up (I personally suggest spaces, but that can be a religious issue ).

        Second, I also have a slightly different approach to fixing this, that I originally started out with. Instead of using a local ArrayJanitor<XMLByte> (or XMLCh) template object to shadow fString, I actually changed the type of fString from a simple pointer to ArrayJanitor<XMLByte>. This means a few more changes here and in TransService.hpp, but it lets ArrayJanitor do more of the work.

        It's really just a matter of style – both approaches work equally well. If you want to keep the changes to a minimum, this patch is the one you want; if you want to clean up a bit, my other fix might be worth considering. Let me know if you're interested, and I will gladly post that as a patch, too.

        Show
        Lee Doron added a comment - By the way, two more notes. First, I noticed inconsistent usage of tabs and spaces for indentation in the transcode() methods. I didn't want to muddy up the patch with whitespace changes, but you might want to clean that up (I personally suggest spaces, but that can be a religious issue ). Second, I also have a slightly different approach to fixing this, that I originally started out with. Instead of using a local ArrayJanitor<XMLByte> (or XMLCh) template object to shadow fString, I actually changed the type of fString from a simple pointer to ArrayJanitor<XMLByte>. This means a few more changes here and in TransService.hpp, but it lets ArrayJanitor do more of the work. It's really just a matter of style – both approaches work equally well. If you want to keep the changes to a minimum, this patch is the one you want; if you want to clean up a bit, my other fix might be worth considering. Let me know if you're interested, and I will gladly post that as a patch, too.
        Hide
        Alberto Massari added a comment -

        Fix is in SVN. Please verify.

        Show
        Alberto Massari added a comment - Fix is in SVN. Please verify.
        Hide
        Lee Doron added a comment -

        I see you implemented my other suggestion, and, interestingly, your code is almost identical to mine! There is one tip I can pass along. I see you disabled a Microsoft warning in Xerces_autoconf_config.msvc.hpp. The warning is actually useful in some cases – see http://www.unknownroad.com/rtfm/VisualStudio/warningC4251.html , http://support.microsoft.com/kb/q168958/ , http://www.eggheadcafe.com/microsoft/VC-Language/30952961/a-solution-to-warning-c4251--class-needs-to-have-dllinterface.aspx , and a few StackOverflow entries. Apparently, there's no perfect solution for this issue, but I avoided the warning by adding the following lines to TransService.hpp, just before the declaration of class TranscodeToStr:

        // Perform explicit instantiations of the ArrayJanitor template as used by
        // classes TranscodeToStr and TranscodeFromStr, respectively, with the
        // XMLUTIL_EXPORT specifier. This forces all members of the class to be
        // generated (and exported if appropriate), and avoids MSVC warning C4251.
        template class XMLUTIL_EXPORT ArrayJanitor<XMLByte>;
        template class XMLUTIL_EXPORT ArrayJanitor<XMLCh>;

        However, I now notice that I left out using something like the EXPIMP_TEMPLATE macro that Microsoft has in their sample code. Perhaps, being much more familiar with the Xerces code than I am, you can decide on the best approach.

        Anyway, this certainly does fix the memory leak. Thanks, and Happy New Year!

        Show
        Lee Doron added a comment - I see you implemented my other suggestion, and, interestingly, your code is almost identical to mine! There is one tip I can pass along. I see you disabled a Microsoft warning in Xerces_autoconf_config.msvc.hpp. The warning is actually useful in some cases – see http://www.unknownroad.com/rtfm/VisualStudio/warningC4251.html , http://support.microsoft.com/kb/q168958/ , http://www.eggheadcafe.com/microsoft/VC-Language/30952961/a-solution-to-warning-c4251--class-needs-to-have-dllinterface.aspx , and a few StackOverflow entries. Apparently, there's no perfect solution for this issue, but I avoided the warning by adding the following lines to TransService.hpp, just before the declaration of class TranscodeToStr: // Perform explicit instantiations of the ArrayJanitor template as used by // classes TranscodeToStr and TranscodeFromStr, respectively, with the // XMLUTIL_EXPORT specifier. This forces all members of the class to be // generated (and exported if appropriate), and avoids MSVC warning C4251. template class XMLUTIL_EXPORT ArrayJanitor<XMLByte>; template class XMLUTIL_EXPORT ArrayJanitor<XMLCh>; However, I now notice that I left out using something like the EXPIMP_TEMPLATE macro that Microsoft has in their sample code. Perhaps, being much more familiar with the Xerces code than I am, you can decide on the best approach. Anyway, this certainly does fix the memory leak. Thanks, and Happy New Year!

          People

          • Assignee:
            Alberto Massari
            Reporter:
            Lee Doron
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development