Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

Patch against crash on Windows x64

2010-05-19
2013-04-29
  • Hi developer(s),

    for the past weeks I have been search for the cause of a crash on Windows. Finally, I could identify the problem and find a solution. The destruction of OptionList causes the crash. My specs on Windows are Visual Studio 2008 w/ SP1, Target Architecture x64. The host OS is Windows 7 x64. The following example works fine for me on Linux, but will crash on Windows when leaving the try-block:

      try {
        jace::StaticVmLoader vJaceJNILoader(JNI_VERSION_1_4);
        jace::OptionList vOptionList;
        vOptionList.push_back(jace::ClassPath(aClassPath));
        jace::helper::createVm(vJaceJNILoader, vOptionList, true);
      } catch(...) {
    

    more precisely, I could identify the following code to crash:

      jace::OptionList* vOptionList = new jace::OptionList;
      delete vOptionList;
    

    After adding a virtual destructor to OptionList, everything is fine on Windows and Linux. Please review the following patch, and apply it if it seems ok to you:

    Index: source/c++/include/jace/OptionList.h
    ===================================================================
    --- source/c++/include/jace/OptionList.h    (revision 39)
    +++ source/c++/include/jace/OptionList.h    (working copy)
    @@ -78,6 +78,12 @@
       JACE_API OptionList();
    
       /**
    +   * Destroys the OptionList.
    +   *
    +   */
    +  virtual JACE_API ~OptionList();
    +
    +  /**
        * Adds a clone of option to the list.
        *
        */
    Index: source/c++/source/jace/OptionList.cpp
    ===================================================================
    --- source/c++/source/jace/OptionList.cpp   (revision 39)
    +++ source/c++/source/jace/OptionList.cpp   (working copy)
    @@ -33,6 +33,9 @@
     OptionList::OptionList() : options() {
     }
    
    +OptionList::~OptionList() {
    +}
    +
     void OptionList::push_back( const Option& option ) {
       OptionPtr ptr( option.clone() );
       options.push_back( ptr );
    

    All the best,

       Mario

     
  • Strange, sourceforge wrongly formatted my post. In my text editor (where I composed it) it is correctly formatted, so I must assume its the forum that actually breaks the code blocks? Here its again:

    Index: source/c++/include/jace/OptionList.h
    ===================================================================
    --- source/c++/include/jace/OptionList.h    (revision 39)
    +++ source/c++/include/jace/OptionList.h    (working copy)
    @@ -78,6 +78,12 @@
       JACE_API OptionList();
    
       /**
    +   * Destroys the OptionList.
    +   *
    +   */
    +  virtual JACE_API ~OptionList();
    +
    +  /**
        * Adds a clone of option to the list.
        *
        */
    Index: source/c++/source/jace/OptionList.cpp
    ===================================================================
    --- source/c++/source/jace/OptionList.cpp   (revision 39)
    +++ source/c++/source/jace/OptionList.cpp   (working copy)
    @@ -33,6 +33,9 @@
     OptionList::OptionList() : options() {
     }
    
    +OptionList::~OptionList() {
    +}
    +
     void OptionList::push_back( const Option& option ) {
       OptionPtr ptr( option.clone() );
       options.push_back( ptr );
    
     
  • Gili Tzabari
    Gili Tzabari
    2010-05-19

    Hi Mario,

    I don't see why a virtual destructor would help because OptionList does not have a superclass. There must be something else going on… Can you please produce a minimal testcase that causes a crash on your end?

    jace::OptionList* vOptionList = new jace::OptionList;
    delete vOptionList;
    

    does not crash on my end.

    Thanks,
    Gili

     
  • Hi Gili,

    about the "virtual" you might be right, I always make destructors virtual when I don't know if somebody derives from them. A non-virtual destructor might solve the problem, too.
    About the crash at all, it is very hard to debug (see at the bottom of this post). My first assumption was the crash happening in the destructor of Option, not OptionList. And in case Option doesn't have an explicit destructor either, it could actually be both of them, i.e. the following code crashed for me in the line "push_back(Option)". I could follow the backtrace to some line saying "~Option()", which I guess is invoked from the autopointer being copied in the vector?

    jace::OptionList vOptionList;
    vOptionList.push_back(jace::ClassPath(std::string("/some/path/with/file.jar"));
    

    However, the identical code runs fine in a simple test program called "show_inf" from the Bio-Formats library, where its not called in a class constructor but rather in a function invoked from main. I don't know as to whether the problem is related to being called in the constructor. But I use a fairly simple setup:

    class A {
      A();
    }
    A::A() {
      // create jace, will crash
    }
    class B : public A {
      B();
    }
    B::B() : A() {
      // no jace related code
    }
    

    When jace gets created in the constructor of A, I get a crash on Windows when the constructor is left due to OptionList being destroyed.

    There is another problem, I cannot run a debug build of jace. The issue is, when I link my program against a debug build of jace, it will not find jace.util.ShutdownHook in jace-runtime.jar. The same program, linked against the release build of jace, works fine. Since my crash in Option or OptionList is happening inside jace.dll, it is really hard to debug!

    Any help is appreciated (though for now, making sure that Option and OptionList have a destructor seems a valid solution for me)

    Mario

     
  • Hi Gili,

    some more things I wanted to add:

    (1) I am using boost-1.42

    (2) The problem linking against a debug-build of jace is not in my program. If I make a debug build of my program which links against the release version of jace, everything is fine. If it links against the debug version of jace, it won't find jace.util.ShutdownHook in jace-runtime.jar.

    (3) Actually, my problems with Option and OptionList could be related. It was not obvious from my previous email, I had two types of crashes: one from inserting an Option into an OptionList with push_back(), and one from the OptionList being destructed. The first issue, I could circumvent by creating Options with new, inserting these into the list, and not destructing the pointers explicitly (which creates a small memory leak which I ignore). The second issue I could circumvent by adding a destructor to OptionList.
    So, this does not work for me:

    try {
      jace::OptionList vOptionList;
      jace::Option vOption(jace::ClassPath(std::string("/some/path/with/file.jar"))
      vOptionList.push_back(vOption); // crash somewhere below ~Option()
     } catch(...) {
    

    the following works better, but still crashes:

    try {
      jace::OptionList vOptionList;
      jace::Option* vOption = new jace::ClassPath(std::string("/some/path/with/file.jar"));
      vOptionList.push_back(*vOption); // no crash
     } catch(...) { // crash when OptionList is destructed
    

    whereas this fully works without a problem:

    try {
      jace::OptionList* vOptionList = new jace::OptionList;
      jace::Option* vOption = new jace::ClassPath(std::string("/some/path/with/file.jar"));
      vOptionList->push_back(*vOption); // no crash
     } catch(...) { // no crash, since no destructor called?
    

    Also, adding a destructor to OptionList solves the issue completely.

    Does that help? All the best, Mario

     
  • sorry, there is a copy-paste-error in the first example, I meant:

    try {
      jace::OptionList vOptionList;
      jace::Option vOption(std::string("/some/path/with/file.jar"));
      vOptionList.push_back(vOption); // crash somewhere below ~Option()
     } catch(...) {
    
     
  • Gili Tzabari
    Gili Tzabari
    2010-05-20

    Hi Mario,

    - Jace not being able to find ShutdownHook:

    In my experience this happens because your classpath does not contain jace-runtime.jar as you assume it does. For example, I ran into this problem because the current working directory under debug mode was different than under release mode. This, in turn, caused my classpath to point to the wrong directory and jace-runtime.jar could not be found. I suggest comparing your debug and release configurations. I'm sure you'll find a problem.

    - Testcase:

    I think you better send me a full-fledged testcase because I cannot reproduce a crash with your code sniplets. I do see one culprit though: you should never mix debug and release builds. The code will compile and run but very often you will end up with weird behavior and unexpected crashes. The problem is that your application will have both debug and release versions of the system libraries in memory, each assuming that it's the only instance in memory. Each library contains some static state it assumes applies system-wide. For example, if you allocate memory in one library (your main application uses debug mode) but then deallocate that same memory in another library (jace uses release mode) then the library doing destructing will not recognize the memory block being destroyed and assume it is invalid (since it never allocated it in the first place). You will then end up with a crash or some other weird behavior.

    This is a guess on my part but I've run into this problem enough times in the past to recognize it. Anytime my application is doing something that should be impossible (stack-traces going haywire, deallocating valid memory crashing, etc) the first thing I do is check my linking options and sure enough I find I have made this mistake.

    Let me know if this help :)

    Gili

     
  • Hi Gili,

    thanks for the quick reply, I appreciate your help!

    - Jace not being able to find ShutdownHook:

    I use a single, full path as the argument for classpath, and that is the path to jace-runtime.jar. For both the debug- and release-builds, I have a textlog that prints the argument to jace::ClassPath, and in both cases its identical. Also, I tried not to use jace::ClassPath but jace::CustomOption("-Djava.class.path=…") which again works fine for the release-build of jace, but not for a debug-build of jace. But, as long as jace works for me, all of this is no big issue :-)

    - Testcase:

    I agree, I'm not a fan of mixing debug- and release-libs either! None of my problems where reported for a mixed build. Actually, sadly enough, the mixed build works, the pure debug build doesn't :-(

    I will try to create a testcase for you, but I can't promise anything. The reason is, I have double-checked your jace code, and I can not see any obvious errors. Creation of the boost::sharedptr seems fine, and you don't call delete on anything explicitly. Honestly, I don't know whats going on (and I'm not a fan of Visual Studio - the code works fine on Linux)! There is one more thing I will do, I will try Intel Compiler 11.1 and see if the resulting executable works or crashes on Windows. Then we have some additional variance in the test, to see if not Visual Studio is the problem. If I find a simple testcase I will of course send it. If not, adding the destructor doesn't hurt, and solves the problem for me :-)

    All the best,

       Mario

     
  • Gili Tzabari
    Gili Tzabari
    2010-05-20

    Mario,

    The only way I can help you further is if you send me compilable source-code to play with. It doesn't even have to be a minimal testcase. Just send me *something* that crashes on your end and I'll try debugging it on my end. :)

    Thanks,
    Gili

     
  • Michael
    Michael
    2010-12-22

    I am having similar problems using jace 1.1.1.  I am setting up the jvm in a constructor of a class like Mario is.  I have a minimal test case which shows the problem.  The debug build will run fine but the release build will crash when it destructs the OptionList.  I also figured out that this only happens if you enable Buffer Security Check option in release.  If that is disabled then the crash doesn't happen.  It doesn't look like I can upload the source so here it is:

    manager.h:

    #ifndef MANAGER_H
    #define MANAGER_H
    namespace jace
    {
    #ifdef _MSC_VER
        class Win32VmLoader;
    #else
        class UnixVmLoader;
    #endif
        class OptionList;
    }
    class Manager
    {
    public:
        //! Constructor will initialize the connection to the database and
        //! sets up the JVM.
        Manager();
        //! Destructor will close down connection and unload the JVM
        virtual ~Manager();
    private:
    #ifdef _MSC_VER
        jace::Win32VmLoader* mLoader;
    #else
        jace::UnixVmLoader* mLoader;
    #endif
    };
    #endif
    

    manager.cpp:

    #include "Manager.h"
    #include <jace/JNIHelper.h>
    #ifdef _MSC_VER
    #include <jace/Win32VmLoader.h>
    using jace::Win32VmLoader;
    #else
    #include <jace/UnixVmLoader.h>
    using jace::UnixVmLoader;
    #endif
    #include <jace/OptionList.h>
    using jace::OptionList;
    #include <iostream>
    #include <string>
    Manager::Manager()
    : mLoader(0)
    {
        std::cout << "Manager() enter." << std::endl;
        // get the JVM path
        std::string jvmPath;
        const char* javaHome = getenv("JAVA_HOME");
        if (javaHome)
            jvmPath = javaHome;
        else
            jvmPath = "C:/Program Files (x86)/Java/jdk1.6.0_12";
        jvmPath += "/jre/bin/client/jvm.dll";
    #ifdef _MSC_VER
        mLoader = new Win32VmLoader(jvmPath, JNI_VERSION_1_6);
    #else
        mLoader = new UnixVmLoader(jvmPath, JNI_VERSION_1_6);
    #endif
        OptionList options;
        options.push_back(jace::CustomOption("-Xmx128M"));
        std::string classpath;
        // Jace 1.1.1 has a bug with missing jace-runtime.jar in the classpath: http://sourceforge.net/projects/jace/forums/forum/63528/topic/3551835
        // It's fixed in the SVN revision 37 of upcoming version 1.1.2: https://jace.svn.sourceforge.net/svnroot/jace
        // As a workaround, we can change environment variable or manually add that to the classpath when creating VM
        std::string jaceRuntime;
        const char* jaceHome = getenv("JACE_HOME");
        if (jaceHome)
            jaceRuntime = jaceHome;
        else
            jaceRuntime = "C:/jace1.1.1/release";
        jaceRuntime += "/lib/jace-runtime.jar;";
        classpath += jaceRuntime;
        options.push_back(jace::ClassPath(classpath));
        // Create the VM will throw runtime_error if it fails
        jace::helper::createVm(*mLoader, options, false);
        std::cout << "Manager() leave." << std::endl;
    }
    Manager::~Manager()
    {
        std::cout << "~Manager() enter." << std::endl;
        delete mLoader;
        std::cout << "~Manager() leave." << std::endl;
    }
    

    main.cpp:

    #include "manager.h"
    #include <iostream>
    int main(int, char**)
    {
        Manager manager;
        std::cout << "We have constructed." << std::endl;
        return(0);
    }
    

    My current solution is to do the same thing as Mario and leak the OptionList.

     
  • Thanks for the nice review, this will be very helpful for debugging! One small comment: I don't have to leak the OptionList. For me, the destructor works fine as soon as Option() and OptionList() have virtual destructor methods.
    BTW: did you try if it works with Visual Studio 2010? Maybe its a compiler-bug?

     
  • Gili Tzabari
    Gili Tzabari
    2010-12-23

    Hi Michael,

        The testcase seems to run fine under the trunk version of Jace. Please try this newer version.

    Gili

     
  • Michael
    Michael
    2010-12-23

    I will try to do the testing using Jace trunk.  We haven't moved to VS 2010 yet but it is possible a compiler bug so newer VS might also solve it.  Also note that disabling Buffer Security Check will allow the release build to run without problems.

     
  • Michael
    Michael
    2010-12-30

    Updating to the latest trunk of Jace appears to fix the issue.  Any plans to create a 1.2 release?

    On a side note the Jace documentation should be updated to reflect the move to Jace.h from JNIHelper.  Wasn't too hard to figure out based on the recent commit messages but would be good to get it updated.

     
  • Gili Tzabari
    Gili Tzabari
    2011-01-01

    Yes, I'd like to make an official 1.2 release but as you've stated the documentation needs to be updated first. I'll try to work towards this in the coming weeks.

    Patches are welcome (and will speed this process).

     
  • Michael
    Michael
    2011-01-17

    Just wanted to update with my latest findings.  The simple test case does work fine with the latest build of jace, but my production code which pulls in other C++ libraries and a large JAR was still failing.  Strangely leaking the OptionList again seemed to fix the issue.  I was starting to get random crashes in other code though when STL containers were being deleted.  I tracked this down to the _SECURE_SCL setting in os_dep.h.  I currently am stuck with using the default setting for that in my other C++ libraries that I'm using and so when I started including Jace.h it defined a value for it which isn't the default and so the includes after it were incorrect.  I have currently commented out that define in os_dep.h and everything is fine.

    So if you don't know what _SECURE_SCL is and does then you likely will be impacted by this issue.  I would like to change my code to use the setting that os_dep.h uses but so far I haven't been successful switching it over.  Since _SECURE_SCL is a Visual Studio thing that is why you normally see this issue only on Windows.

    Hope that helps,

    Michael

     
  • Gili Tzabari
    Gili Tzabari
    2011-01-22

    I could have sworn I had replied to this post, but I can't find any trace of it. Anyway, I tried removing:

    #ifndef _SECURE_SCL
    #define _SECURE_SCL 0
    #endif

    from os_dep.h and it looks like everything builds just fine. Can you please confirm this on your end? If no one reports any problems with this then I will remove it in the official release.

    Thanks,
    Gili

     
  • Gili Tzabari
    Gili Tzabari
    2011-01-24

    I've committed this fix to trunk (revision 66 or newer).