search

serialport / bindings-cpp

The C++ bindings for the node serialport project.
MIT License
19 stars 39 forks source link

fix: infinite recursion in unix-read #164

Open skadisch opened 9 months ago

skadisch commented 9 months ago

Fixes #162

According to man 2 read:

On success, the number of bytes read is returned (zero indicates end of file)...

According to node fs:

If the file is not modified concurrently, the end-of-file is reached when the number of bytes read is zero.

If I understand this correctly (maybe there are some edge cases in the serialport read API?), we should never recurse if read returns zero.

Also, the @serialport/stream package (correctly) contains checks for zero-sized reads here, which could never be the case if we keep recursing on zero.

I had the problems described in #162 with a USB serial port (/dev/ttyUSB0). When I unplugged it during runtime, my program went into this infinite recursion and crashed due to memory. With this fix, it worked as expected.

Kiliar commented 2 months ago

As I understand, this is an implementation of pulling logic to wait until there is a data to grub. Endless loop is better thou.

skadisch commented 3 weeks ago

Yet, a zero-sized read indicates end of file, or in this case, disconnect of the device. So even if you refactor to a "loop", and add some delays, you will not be able to detect the disconnect. I think this was designed with devices in mind, that don't support hot plugging. But then again, those should never give you zero sized reads...

Kiliar commented 3 weeks ago

On linux, after disconnect the file will just disappear. But while connection is established and no data is receiving, there will be just an empty file with 0 bytes. In this case recursion will go as deep as you have available memory and crush your app, unless you receive some data. I've refactored this part to be while (true) {} loop with some delay & exit conditions, it solved high cpu usage & out of memory crushes.

skadisch commented 3 weeks ago

Where did you solve it? In this case, this would be in violation of the documentation, as cited in my original post. What exactly are the exit conditions you use? What kind of serial port shows this behaviour?

Kiliar commented 3 weeks ago

I fixed it in my personal project, by pre-building library and replacing this file with fixed one. I can create PR with my solution, so you can take a look.

Kiliar commented 3 weeks ago

https://github.com/serialport/bindings-cpp/pull/175