Review my first run at a Rust binding for the gensio library

I have a library I support, I've been working on a binding at GitHub - cminyard/rust-gensio: Rust bindings to the gensio library that I've been getting ready. Since this site has an offer of a code review, I mean, how can I turn that down.

From the (currently woefully inadequate) README:

gensio provides consistent access to a number of different types of
stream (and packet) I/O, things like serial ports, TCP, UDP, stdio,
files, ptys, sound, and more. It does this through a single
consistent API.

It works on Linux, BSD, MacOS, and Windows.

It also has filter gensios which can do processing on a stream and
stacked on top of other gensios. Things like telnet, encryption,
authentication, and others.

I'm new to rust, and any pointers on how to do things would be appreciated. A lot more documentation needs to be done. The binding is fairly complete, but I'm sure it could be improved.

-corey

Look no further than the supporting C code for mistakes:

  • In ghelpers.c: upon the two early returns (in both non-trivial functions), va_end() is never called, which is undefined behavior. (The C standard prescribes that every use of va_start() and va_copy() must be paired with a corresponding call to va_end().)
  • Similarly, the "ownership" (for the lack of better terminology) of varargs in oshelpers.c isn't clear, either.

Issues with the Rust part:

  • the giant lists of error and control codes in lib.rs, e.g. GE_NOERR etc., should be enums. Rust is not C: we have proper namespaces, associated items, and other mechanisms for grouping code. It's unnecessary to create long lists of individual constants with the same repetitive prefix. (You should run Clippy which would catch this kind of mistake.)
  • There's a lot of C-style "if error then free resources" error handling, which is not unwind safe (i.e., prone to leaking upon unexpected panics). Examples are 1, 2. You should instead be wrapping resources into RAII handlers that implement Drop.
  • The Weak thing is weird. Actually, I'd go so far as to say it's a footgun. People won't remember that they need to keep the callback around once they passed it to the library, and this will lead to hard-to-debug errors, especially so because the symptoms won't be loud (no crash or panic or even error messages being logged), the user will simply not see any callbacks fired if they forget to keep an Rc. You should really re-design this part in a more sensible and user-friendly way. A good API is not one that can be used correctly but one that can't be used incorrectly.
  • In general, there is a lot of repetition, unsafe sprinkled everywhere, and tangled data structures with pointers referring each other left and right. That's bound to cause memory management errors and undefined behavior.
  • There's a bunch of #[allow(inproper_ctypes)] directive in this module. That lint shouldn't be taken lightly; chances are silencing it leads to UB due to the non-guaranteed ABI of Rust types. (I haven't looked at all uses because it's way too much code for me to digest in one go, but it's suspicious to say the least.)

Trying to wrap a C library with complicated ownership structure is likely not a great first project. It necessarily entails using unsafe and with that, an inevitable need for a deep understanding of the language arises. You should probably start with something simpler, ideally something not involving unsafe.

Look no further than the supporting C code for mistakes:

  • In [ghelpers.c] upon the two early returns (in both non-trivial functions), va_end() is never called, which is undefined behavior. (The C standard prescribes that every use of va_start() and va_copy() must be paired with a corresponding call to va_end().)
  • Similarly, the "ownership" (for the lack of better terminology) of varargs in oshelpers.c isn't clear, either.

Oops, that was dumb.

Issues with the Rust part:

  • the giant lists of error and control codes in lib.rs, e.g. GE_NOERR etc., should be enums. Rust is not C: we have proper namespaces, associated items, and other mechanisms for grouping code. It's unnecessary to create long lists of individual constants with the same repetitive prefix. (You should run Clippy which would catch this kind of mistake.)

Clippy didn't find this. But yes, you are right. I should have done
that sooner.

  • There's a lot of C-style "if error then free resources" error handling, which is not unwind safe (i.e., prone to leaking upon unexpected panics).

I re-reviewed these and found a couple of issues, but in general, that's
what this code is doing. It's wrapping the raw types in Rust types and
implementing Drop for them. There's just a window where the raw type is
there and it's not yet in a Rust type, and I think all of them are safe
now.

  • The Weak thing is weird. Actually, I'd go so far as to say it's a footgun. People won't remember that they need to keep the callback around once they passed it to the library, and this will lead to hard-to-debug errors, especially so because the symptoms won't be loud (no crash or panic or even error messages being logged), the user will simply not see any callbacks fired if they forget to keep an Rc. You should really re-design this part in a more sensible and user-friendly way. A good API is not one that can be used correctly but one that can't be used incorrectly.

I don't like this either, I agree with what you say. But if the user
passes in a type with a reference to the object, then it has a circular
reference and it will leak memory silently. That seems worse.

I could have the user pass in an Arc and convert it to Weak in the code,
but that seems worse, as there's not explicit thing about it being Weak.

I'm not sure how to fix this. The code could complain loudly. But it
would be better if the user couldn't make this mistake. I'm not sure
how to do it. I'm still poking around.

  • In general, there is a lot of repetition, unsafe sprinkled everywhere, and tangled data structures with pointers referring each other left and right. That's bound to cause memory management errors and undefined behavior.
  • There's a bunch of [#[allow(inproper_ctypes)] directive in this module]. That lint shouldn't be taken lightly; chances are silencing it leads to UB due to the non-guaranteed ABI of Rust types. (I haven't looked at all uses because it's way too much code for me to digest in one go, but it's suspicious to say the least.)

Those are for opaque C types. I'm not sure what else to do.

Trying to wrap a C library with complicated ownership structure is likely not a great first project. It necessarily entails using unsafe and with that, an inevitable need for a deep understanding of the language arises. You should probably start with something simpler, ideally something not involving unsafe.

Well, yes, probably. I've done this for other languages, but Rust is,
well, different.

Thanks for your response.

-corey

1 Like

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.