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