NeuronRobotics / nrjavaserial

A Java Serial Port system. This is a fork of the RXTX project that uses in jar loading of the native code.
Other
345 stars 143 forks source link

When RXTX.run() exits, C code should no longer access the eis structure #59

Closed cxbrooks closed 8 years ago

cxbrooks commented 8 years ago

madhephaestus suggested that I make an issue for this and add a comment or commit.

Under Mac OS X, running a Ptolemy model (SerialHelloWorld.xml )model was crashing Java in readArray(). I don't have a small test case for this, and my changes are more of a workaround than a fix.

The model opens two ports, writes to one port and halts after 5 seconds.

The code does not use the RXTXListener facility, instead it creates reader and writer threads.

The cause is that when the Java RXTXPort.run() method exits, a C struct named eis goes out of scope. If the drain thread tries to access eis after a timeout, then a segfault occurs.

What's happening is that in nrjavaserial/src/main/c/src/SerialImp.c, readArray() is calling read_byte_array() and read_byte_array() is crashing when eis is accessed:

        while( bytes < length &&  count++ < 20 ) /* && !is_interrupted( eis ) )*/
        {
                if (timeout >= 0) {
                        now = GetTickCount();
                        if ( now-start >= timeout )
                        {
--->                            eis->eventflags[SPE_DATA_AVAILABLE] = flag;
                                return bytes;
                        }
                }

The reason that it is crashing here is because eis is allocated in eventLoop() in SerialImp.c by struct event_info_struct eis:

JNIEXPORT void JNICALL RXTXPort(eventLoop)( JNIEnv *env, jobject jobj )
{
#ifdef WIN32
        int i = 0;
#endif /* WIN32 */
        struct event_info_struct eis;
        eis.jclazz = (*env)->GetObjectClass( env, jobj );
        eis.env = env;
        eis.jobj = &jobj;
        eis.initialised = 0;

        ENTER( "eventLoop\n" );
        if ( !initialise_event_info_struct( &eis ) ) goto end;
        if ( !init_threads( &eis ) ) goto end;

init_threads() sets the Java eis object:

        report("init_threads: get eis\n");
        jeis  = (*eis->env)->GetFieldID( eis->env, eis->jclazz, "eis", "J" );
        report("init_threads: set eis\n");
        (*eis->env)->SetLongField(eis->env, *eis->jobj, jeis, ( size_t ) eis );

nrjavaserial/src/main/java/gnu/io/RXTXPort.java invokes eventLoop()

        /** a pointer to the event info structure used to share information 
            between threads so write threads can send output buffer empty   
            from a pthread if need be.     

            long for 64 bit pointers.                                                                                                    
        */
        long eis = 0;

...

        /** Process SerialPortEvents */
        native void eventLoop();

...

        /**                                                                                                                              
        *  run the thread and call the event loop.                                                                                      
        */
                public void run()
                {
                        if (debug)
                                z.reportln( "RXTXPort:MontitorThread:run()");
                        monThreadisInterrupted=false;
                        eventLoop();
                        if (debug)
                                z.reportln( "eventLoop() returned");
                }

So, if the run() method ends, the eis automatic variable in eventloop() goes out of scope.

cxbrooks commented 8 years ago

I submitted a pull request #58, below is the most important part of change:

diff --git a/src/main/c/src/SerialImp.c b/src/main/c/src/SerialImp.c
index fdd1c9b..cce8fd1 100644
--- a/src/main/c/src/SerialImp.c
+++ b/src/main/c/src/SerialImp.c
@@ -3074,7 +3074,13 @@ int read_byte_array( JNIEnv *env,
            now = GetTickCount();
            if ( now-start >= timeout )
            {
-                               eis->eventflags[SPE_DATA_AVAILABLE] = flag;
+                                struct event_info_struct *eis2 = ( struct event_info_struct * )
+                                    get_java_var_long( env, *jobj,"eis","J" );
+                                if (eis2 == NULL) {
+                                        report("read_byte_array(): eis was null, this can happen if reading after RXTXPort.run() returns while reading.");
+                                } else {
+                                        eis->eventflags[SPE_DATA_AVAILABLE] = flag;
+                                }
                                return bytes;
                }
                }