search

sgrif / mysqlclient-sys

Rust bindings for libmysqlclient
Apache License 2.0
37 stars 31 forks source link

Test failures on various architectures #19

Closed kpcyrd closed 5 months ago

kpcyrd commented 4 years ago

We've encountered test failures in debian on the following architectures:

There might be something wrong with the bindings on these architectures.

sgrif commented 4 years ago

Thanks for letting me know. I don't have access to any of those architectures, so there's not a ton I can do. However, I'd be happy to accept a PR with the relevant changes

plugwash commented 1 year ago

I ground through the tests, looking at both the rust code and the "mysql" (actually mariadb) headers on my system. I have produced a patch that makes the tests pass on i386. I have not yet tested said patch on other architectures.

I came to the following conclusions.

  1. As has been my past experiance, bindgen generated tests are totally un-portable and required significant manual reworking and require significant manual reworking to make them portable.
  2. For the most part the structure definitions were translated from C in a portable way. I did however find one place where this was not the case. The old version of bindgen had translated a union from C to Rust in a non-portable way. I patched that and continued,
  3. There seemed to be some quite significant differences between the headers on my system and the rust bindings, that were not explained by architecture issues. I strongly suspect that there are significant soundness issues using these bindings with mariadb on any architecture.

I have attatched the patch, note that so-far this has only been tested on i386, it may well require some tweaking to work on other architectures.

fix-tests-32-bit.patch.txt

weiznich commented 1 year ago

@plugwash Thanks for providing this patch. Could you open a PR with this changes? I think it should be fine to merge that as soon as we are sure that it passes tests on x86_64 and at least one other architecture.

There seemed to be some quite significant differences between the headers on my system and the rust bindings, that were not explained by architecture issues. I strongly suspect that there are significant soundness issues using these bindings with mariadb on any architecture.

The generated bindings are really old, but they seem to work fine as far as I can tell. Now that does not necessarily mean that they are 100% correct nor perfect. I'm certainly open for improvements there, but I would like to see a working CI setup first before doing large changes to this crate. I personally do currently not have the capacity for that, so I'm happy for help there.

weiznich commented 5 months ago

This is fixed by #37 by only allowing to use the prebuilt bindings on supported architectures and otherwise suggest to use the buildtime_bindgen feature to generate matching bindings at build time.