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.

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

fix: infinite recursion in unix-read #164

Closed

Conversation

skadisch
Copy link

@skadisch skadisch commented Dec 6, 2023

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
Copy link

Kiliar commented Jul 1, 2024

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
Copy link
Author

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
Copy link

Kiliar commented Aug 23, 2024

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
Copy link
Author

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
Copy link

Kiliar commented Aug 23, 2024

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
Copy link

Kiliar commented Aug 23, 2024

#175

@reconbot
Copy link
Member

I believe it is quite common with non blocking reads to get 0 bytes. Also currently the contract of the binding is to always return at least 1 byte. Your runaway process in #162 is a different issue. We might not be getting an error when we should be.

@reconbot reconbot closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Recursion+memory leak in unixRead()
3 participants