search

haskell / c2hs

c2hs is a pre-processor for Haskell FFI bindings to C libraries
http://hackage.haskell.org/package/c2hs
Other
197 stars 50 forks source link

Boolean struct members are incorrectly marshalled #257

Closed drone29a closed 3 years ago

drone29a commented 3 years ago

The struct field getter for C2HS (src/C2HS/Gen/Bind.hs:1715) assumes a C bool struct member is stored as a CInt (4 bytes), when C bool values on many platforms use 1 byte. This results in corrupted boolean values on such platforms, at a minimum, and may cause a program using C2HS to crash if the process attempts to read memory that it cannot access.

A longterm fix would use platform information about the size of a bool to select the correct Haskell FFI type to use. In the near term, it may be worthwhile to change the CInt to CBool (https://hackage.haskell.org/package/base-4.10.1.0/docs/Foreign-C-Types.html#t:CBool, introduced in 2017 with GHC 8.2.1) or use a 1-byte type available in earlier versions of GHC such as CChar. While not defined in any standards, in practice it seems a C bool is most often stored in a single byte.

I'm happy to submit a PR, but as there are several outstanding PRs, I would like to at first discuss the issue with any maintainers before writing a solution. Thanks for any info!

deech commented 3 years ago

Hi, I unfortunately was not notified of any issues or PRs until today when someone directly @'ed me. In any case, we should at least be able to test the size of a bool locally and generate either a CInt or CChar. The only concern is that will break codebases that now depend on a bool marshalling to CInt.

I agree that using CBool is the cleanest solution but I'm not sure how many people are still using c2hs on GHC's older than 2017.

Any thoughts?

drone29a commented 3 years ago

Hi deech, Oh no, I'm glad to hear there was only a notifications issue but sorry you have to deal with a backlog.

I think the right approach for C2HS is to maintain backwards compatibility as much as possible. After looking at the available C types in Foreign.C.Types again, using CUChar (rather than CChar as I originally suggested) for a one-byte representation of C's bool and selecting which C type to use based on the bool size test seems like a good plan.

There should be no functional difference between CUChar and CBool as both are newtypes wrapping a Word8 and we would still use toBool to convert to Haskell's Bool. A quick peek at the CBool definitions for various classes like Eq seems to show they are derived from Word8. I'm not entirely comfortable reading that code quickly, so it wouldn't hurt to double check.

drone29a commented 3 years ago

Hi @deech, I ran into this problem again. Were you ok with the solution I proposed earlier? I'd like to fix it instead of writing workarounds in my own code.