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 (RTU USB) failure to select() an invalid file descriptor #1

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

jimklimov
Copy link
Member

Implementation of new libusb-based USB RTU for libmodbus, as proposed by @EchterAgo as part of networkupstools/nut#2063 solution, involves direct libusb API communications in the new backend. As such, there is normally no file descriptor to select() for incoming data, as is the case with serial port or a network socket (TCP RTU).

After investigation done in networkupstools/nut#2609 and related tickets (networkupstools/nut#2289, networkupstools/nut#2732) I am convinced that the technical issue is in this backend never initializing ctx->s so it remains -1 from common allocation. The (generally platform-defined) implementations of FD_SET et al expect non-negative integers as file descriptor numbers:

An fd_set is a fixed size buffer. Executing FD_CLR() or FD_SET() with a value of fd that is negative or is equal to or larger than FD_SETSIZE will result in undefined behavior. Moreover, POSIX requires fd to be a valid file descriptor.

While some platforms used for initial development apparently were lax about this for whatever reason, on others later on this use of -1 misfired visibly; I suspect they were made more secure about checking for memory errors (and here we could have had an array underflow, depending on FD_XXX implementation as e.g. macros).

This PR proposes a couple of changes:

These changes were tested in networkupstools/nut#2609 (comment) which collected the device data and did not crash like before.

NOTE: The test Got a list of 3 libusb file descriptors to poll and so did not assign any to ctx->s; this may need more exploration - what those FDs are and if each should be polled. Maybe change the single ctx->s to an array (or to keep code of other RTU's largely unmodified, add the array to backend or even common ctx structure, and use it for select() if e.g. ctx->s==-2)?

@jimklimov jimklimov added the bug Something isn't working label Jan 14, 2025
@jimklimov jimklimov merged commit 512aba6 into networkupstools:rtu_usb Jan 14, 2025
1 check passed
@jimklimov jimklimov deleted the issue-nut-2609 branch January 14, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant