Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use poll in readBytes to fix segfaults with high fd numbers #90

Merged
merged 4 commits into from
Aug 4, 2021
Merged

Use poll in readBytes to fix segfaults with high fd numbers #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c815db
Use poll to fix segfaults with high fd numbers
Jul 13, 2021
ec9d759
Apply suggested changes (3rd)
Jul 15, 2021
1e9b04e
Merge upstream changes from master to feature
Jul 26, 2021
1aff5f9
Remove sleep()
tresf Aug 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 83 additions & 11 deletions src/main/cpp/_nix_based/jssc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@
#include <time.h>
#include <errno.h>//-D_TS_ERRNO use for Solaris C++ compiler

#include <sys/select.h>//since 2.5.0

#ifdef __linux__
#include <linux/serial.h>
#endif
Expand All @@ -42,6 +40,18 @@
#endif
#ifdef __APPLE__
#include <serial/ioss.h>//Needed for IOSSIOSPEED in Mac OS X (Non standard baudrate)
#elif !defined(HAVE_POLL)
// Seems as poll has some portability issues on OsX (Search for "poll" in
// "https://cr.yp.to/docs/unixport.html"). So we only make use of poll on
// all platforms except "__APPLE__".
// If you want to force usage of 'poll', pass "-DHAVE_POLL=1" to gcc.
#define HAVE_POLL 1
#endif

#if HAVE_POLL == 0
#include <sys/select.h>
#else
#include <poll.h>
#endif

#include <jni.h>
Expand Down Expand Up @@ -524,28 +534,90 @@ JNIEXPORT jboolean JNICALL Java_jssc_SerialNativeInterface_writeBytes
return result == bufferSize ? JNI_TRUE : JNI_FALSE;
}

/**
* Waits until 'read()' has something to tell for the specified filedescriptor.
*/
static void awaitReadReady(JNIEnv*env, jlong fd){
#if HAVE_POLL == 0
// Alternative impl using 'select' as 'poll' isn't available (or broken).

//assert(fd < FD_SETSIZE); // <- Might help when hunting SEGFAULTs.
fd_set readFds;
while(true) {
FD_ZERO(&readFds);
FD_SET(fd, &readFds);
int result = select(fd + 1, &readFds, NULL, NULL, NULL);
if(result < 0){
// man select: On error, -1 is returned, and errno is set to indicate the error
// TODO: Maybe a candidate to raise a java exception. But won't do
// yet for backward compatibility.
continue;
}
// Did wait successfully.
break;
}
FD_CLR(fd, &readFds);

#else
// Default impl using 'poll'. This is more robust against fd>=1024 (eg
// SEGFAULT problems).

struct pollfd fds[1];
fds[0].fd = fd;
fds[0].events = POLLIN;
while(true){
int result = poll(fds, 1, -1);
if(result < 0){
// man poll: On error, -1 is returned, and errno is set to indicate the error.
tresf marked this conversation as resolved.
Show resolved Hide resolved
// TODO: Maybe a candidate to raise a java exception. But won't do
// yet for backward compatibility.
continue;
}
// Did wait successfully.
break;
}

#endif
}

/* OK */
/*
* Reading data from the port
*
* Rewrited in 2.5.0 (using select() function for correct block reading in MacOS X)
* Rewritten to use poll() instead of select() to handle fd>=1024
*/
JNIEXPORT jbyteArray JNICALL Java_jssc_SerialNativeInterface_readBytes
(JNIEnv *env, jobject, jlong portHandle, jint byteCount){
fd_set read_fd_set;

// TODO: Errors should be communicated by raising java exceptions; Will break
// backwards compatibility.

jbyte *lpBuffer = new jbyte[byteCount];
jbyteArray returnArray = NULL;
int byteRemains = byteCount;

while(byteRemains > 0) {
FD_ZERO(&read_fd_set);
FD_SET(portHandle, &read_fd_set);
select(portHandle + 1, &read_fd_set, NULL, NULL, NULL);
int result = read(portHandle, lpBuffer + (byteCount - byteRemains), byteRemains);
if(result > 0){
int result = 0;

awaitReadReady(env, portHandle);

errno = 0;
result = read(portHandle, lpBuffer + (byteCount - byteRemains), byteRemains);
if (result < 0) {
// man read: On error, -1 is returned, and errno is set to indicate the error.
// TODO: May candidate for raising a java exception. See comment at begin of function.
}
else if (result == 0) {
// AFAIK this happens either on EOF or on EWOULDBLOCK (see 'man read').
// TODO: Is "just continue" really the right thing to do? I will keep it that
// way because the old code did so and I don't know better.
}
else {
byteRemains -= result;
}
}
FD_CLR(portHandle, &read_fd_set);
jbyteArray returnArray = env->NewByteArray(byteCount);

returnArray = env->NewByteArray(byteCount);
env->SetByteArrayRegion(returnArray, 0, byteCount, lpBuffer);
delete[] lpBuffer;
return returnArray;
Expand Down
31 changes: 12 additions & 19 deletions src/main/java/jssc/SerialPort.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,24 +26,23 @@

import java.io.UnsupportedEncodingException;
import java.lang.reflect.Method;
import java.nio.charset.Charset;

/**
*
* @author scream3r
*/
public class SerialPort {

private SerialNativeInterface serialInterface;
private final SerialNativeInterface serialInterface;
private SerialPortEventListener eventListener;
private long portHandle;
private String portName;
private boolean portOpened = false;
private volatile long portHandle;
private final String portName;
private volatile boolean portOpened = false;
private boolean maskAssigned = false;
private boolean eventListenerAdded = false;

//since 2.2.0 ->
private Method methodErrorOccurred = null;
private volatile Method methodErrorOccurred = null;
//<- since 2.2.0

public static final int BAUDRATE_110 = 110;
Expand Down Expand Up @@ -149,7 +148,7 @@ public boolean isOpened() {
*
* @throws SerialPortException
*/
public boolean openPort() throws SerialPortException {
public synchronized boolean openPort() throws SerialPortException {
if(portOpened){
throw new SerialPortException(this, "openPort()", SerialPortException.TYPE_PORT_ALREADY_OPENED);
}
Expand Down Expand Up @@ -209,7 +208,7 @@ public boolean setParams(int baudRate, int dataBits, int stopBits, int parity) t
*
* @since 0.8
*/
public boolean setParams(int baudRate, int dataBits, int stopBits, int parity, boolean setRTS, boolean setDTR) throws SerialPortException {
public synchronized boolean setParams(int baudRate, int dataBits, int stopBits, int parity, boolean setRTS, boolean setDTR) throws SerialPortException {
checkPortOpened("setParams()");
if(stopBits == 1){
stopBits = 0;
Expand Down Expand Up @@ -238,7 +237,7 @@ else if(stopBits == 3){
*
* @throws SerialPortException
*/
public boolean purgePort(int flags) throws SerialPortException {
public synchronized boolean purgePort(int flags) throws SerialPortException {
checkPortOpened("purgePort()");
return serialInterface.purgePort(portHandle, flags);
}
Expand All @@ -261,7 +260,7 @@ public boolean purgePort(int flags) throws SerialPortException {
*
* @throws SerialPortException
*/
public boolean setEventsMask(int mask) throws SerialPortException {
public synchronized boolean setEventsMask(int mask) throws SerialPortException {
checkPortOpened("setEventsMask()");
if(SerialNativeInterface.getOsType() == SerialNativeInterface.OS_LINUX ||
SerialNativeInterface.getOsType() == SerialNativeInterface.OS_SOLARIS ||
Expand Down Expand Up @@ -556,12 +555,6 @@ private void waitBytesWithTimeout(String methodName, int byteCount, int timeout)
timeIsOut = false;
break;
}
try {
Thread.sleep(0, 100);//Need to sleep some time to prevent high CPU loading
}
catch (InterruptedException ex) {
//Do nothing
}
}
if(timeIsOut){
throw new SerialPortTimeoutException(portName, methodName, timeout);
Expand Down Expand Up @@ -1004,7 +997,7 @@ public void addEventListener(SerialPortEventListener listener, int mask) throws
*
* @throws SerialPortException
*/
private void addEventListener(SerialPortEventListener listener, int mask, boolean overwriteMask) throws SerialPortException {
private synchronized void addEventListener(SerialPortEventListener listener, int mask, boolean overwriteMask) throws SerialPortException {
checkPortOpened("addEventListener()");
if(!eventListenerAdded){
if((maskAssigned && overwriteMask) || !maskAssigned) {
Expand Down Expand Up @@ -1056,7 +1049,7 @@ private EventThread getNewEventThread() {
*
* @throws SerialPortException
*/
public boolean removeEventListener() throws SerialPortException {
public synchronized boolean removeEventListener() throws SerialPortException {
checkPortOpened("removeEventListener()");
if(!eventListenerAdded){
throw new SerialPortException(this, "removeEventListener()", SerialPortException.TYPE_CANT_REMOVE_LISTENER);
Expand Down Expand Up @@ -1085,7 +1078,7 @@ public boolean removeEventListener() throws SerialPortException {
*
* @throws SerialPortException
*/
public boolean closePort() throws SerialPortException {
public synchronized boolean closePort() throws SerialPortException {
checkPortOpened("closePort()");
if(eventListenerAdded){
removeEventListener();
Expand Down