Invalidate RXTXPort.eis when the monitor thread goes away (#267) - #269
Invalidate RXTXPort.eis when the monitor thread goes away (#267)#269thomasnikolay wants to merge 1 commit into
Conversation
RXTXPort.eis holds a pointer to the struct event_info_struct that eventLoop() allocates on the monitor thread's stack. It was only reset after a normal return from eventLoop(), so when the monitor thread ended through the hardware error path the field kept pointing at a stack frame that no longer existed and the next readArray() dereferenced it, killing the JVM with SIGSEGV/EXCEPTION_ACCESS_VIOLATION (issue NeuronRobotics#267). Clear the field on every exit path: from native code in finalize_event_info_struct() (preserving a pending exception, since JNI calls are not allowed while one is in flight) and from Java in a finally block in MonitorThread.run(). The existing NULL check in read_byte_array() then turns the situation into an IOException.
|
@MrDOS Can you please check this PR and if it ok please merge and release it. |
|
I'd first like to understand the problem description a little bit better – I have not really had time to digest it, sorry. I have also thought for a long time now that event info structs should be heap-allocated and pointed to by a private field on the At a higher level, though, and for the same reasons outlined by Twisted's draft AI policy, I am extremely leery of accepting an LLM-authored contribution. This project is LGPL-licensed, not MIT-licensed, and I am wary of accepting code into it which was not knowingly authored in kind. I am also pretty discouraged by the output of the genAI industry at large (both its products and its rhetoric), and so I prefer to stay as far away from it as possible in the things I do in my spare time. All to say that there is a years-long backlog of other PRs and work to do on this project, and I am inclined to spend my very limited time on those human-authored things first. |
Summary
Fixes the JVM crash reported in #267 (
EXCEPTION_ACCESS_VIOLATION/ SIGSEGV inread_byte_arrayafter a serial adapter is unplugged).RXTXPort.eisstores a pointer to thestruct event_info_structthatRXTXPort(eventLoop)declares as a local, i.e. it lives on the monitor thread's stack. Today the field is only reset aftereventLoop()returns normally:When the monitor thread ends through the hardware-error path,
eiskeeps pointing at a stack frame that no longer exists, and the nextreadArray()dereferences it. The hs_err in #267 faults ateis + 8, which iseventflags[SPE_DATA_AVAILABLE](int fd+ oneint) — theeis == NULLguard added in #249 never triggers, because the field is non-NULL garbage.This clears the pointer on every exit path:
MonitorThread.run():finalize_event_info_struct()calls the newclear_java_eis(), which writes0into the Java field. It saves and rethrows a pending exception around the write, because JNI calls are not allowed while an exception is in flight (otherwise an error thrown out of the event loop would be swallowed).With the field zeroed, the existing NULL check in
read_byte_array()turns a post-disconnect read into anIOExceptioninstead of terminating the JVM.Note: the prebuilt native libraries in
src/main/c/resources/native/are not included in this PR; they need to be rebuilt for the fix to take effect on the native side.Verified by building the JAR and the Linux/Windows natives (JDK 11) and exercising a port over a
socatPTY pair: after the monitor thread ends,eisis0and subsequentreadArray()calls raiseIOExceptioninstead of crashing. The original unplug scenario needs real hardware and was not reproduced here.