Unsafe enum usage

[Koka]

Hi @pacman82 , looks like we have an issue in FFI binding which causes UB. Passing mut enum ref across FFI boundaries causes UB in cases when ODBC driver returns value absent in enum.

Details: https://users.rust-lang.org/t/undefined-behaviour-after-unsafe-enum-usage/15572
Related odbc-rs issue: Koka/odbc-rs#66
Related discussion in rust-bindgen: rust-lang/rust-bindgen#667

Looks like it needs to be fixed at lowest level available i.e. in this crate, I'm unsure ho to fix this without harming usability though.

Looks like we could use manual conversion crates like https://crates.io/crates/enum_primitive or declare enums as #[non_exhaustive] rust-lang/rust#44109 when it will be ready. Or just declare everything as constants and don't use enums at all

[pacman82]

Currently I am leaning towards adding missing enum variants, and not merging any new enumerations if they are not exhaustive. I am happy to be convinced otherwise if you think we should take a different course of action.

[Koka]

That's good, but I'm afraid it's insufficient. There is always a chance that we'll miss some enum variants, or we'll have not so standard compliant odbc implementation or buggy driver which return non standard enum values, or even spec will change adding new enum values. We also need to remember that in C world some enums are used as bitflags actually, so ffi call can return mixed bit combination of enum values. Silently triggering UB in those cases is very user unfriendly and unsafe. It's better to perform fair conversion from ffi returned int to enum and if it was unsuccessful we should return error or at least trigger panic.

[mversic]

I'm with @Koka on this issue. Since this is a low level crate interfacing C code we can never be too certain rust invariants are kept when crossing FFI boundary. If we can agree on this I would like to implement the change.

Proposition:

pub type SQLRETURN = SMALLINT;

pub enum SqlReturn {
    SQL_INVALID_HANDLE = -2,
    SQL_ERROR = -1,
    ...
}

impl TryFrom<SQLRETURN> for SqlReturn {
    ...
}

fn SQLGetEnvAttr(...some_args...) -> SQLRETURN;

This means that it would be up to the user to convert returned SQLRETURN to SqlReturn via SqlReturn::from to use the enum provided with this crate. I would rather have TryFrom implemented in odbc-safe crate but orphan rule forbids that.

Are there any other suggestions?

[mversic]

I've found a way for the best of both worlds by using repr[transparent] on newtype

The proposition now looks like this:

#[repr(transparent)]
pub struct SQLRETURN(SMALLINT);    // Newtype pattern, notice how wrapped type is private

pub enum SqlReturn {
    SQL_INVALID_HANDLE = -2,
    SQL_ERROR = -1,
    ...
}

impl TryFrom<SQLRETURN> for SqlReturn {
    ...
}

fn SQLGetEnvAttr(...some_args...) -> SQLRETURN;

This approach is superior because it provides additional type safety. Users can never construct instances of SQLRETURN, only instances of SqlReturn but are forced to interface FFI with SQLRETURN types. Users can't even access the contents of SQLRETURN. By implementing TryFrom<SQLRETURN> for SqlReturn types returned from the FFI can be converted and accessed in the code.

I've updated the PR related to this

[pacman82]

I am still not sure we have a problem here. The ODBC specification allows us to list all enums. Human error aside, we should be able to provide complete enums. I am not aware of an ODBC function whose returnvalues are specified as bitflags. If there is one, I agree, its return type should not be modled as enum. The only remaining problem I see is if a driver or driver manager would not stick to the specification. If not seen a specific instance of this happening. If this is something which appears in practice I would suggest switching the return types to Integers and working with plain constants.

[pacman82]

Reading the links to the orginal issue again. Let's switch to integer return types. I do not think we need strict newtypes though.

[mversic]

what do you mean? to dismantle the SqlReturn enum and expose it's variants as constants?

[jpastuszek]

I have just run into SIGSEGV with INTERVAL data type in a query.

#if (ODBCVER >= 0x0300)
/* interval code */
#define SQL_CODE_YEAR                           1
#define SQL_CODE_MONTH                          2
#define SQL_CODE_DAY                            3
#define SQL_CODE_HOUR                           4
#define SQL_CODE_MINUTE                         5
#define SQL_CODE_SECOND                         6
#define SQL_CODE_YEAR_TO_MONTH                  7
#define SQL_CODE_DAY_TO_HOUR                    8
#define SQL_CODE_DAY_TO_MINUTE                  9
#define SQL_CODE_DAY_TO_SECOND                  10
#define SQL_CODE_HOUR_TO_MINUTE                 11
#define SQL_CODE_HOUR_TO_SECOND                 12
#define SQL_CODE_MINUTE_TO_SECOND               13

#define SQL_INTERVAL_YEAR                                       (100 + SQL_CODE_YEAR)
#define SQL_INTERVAL_MONTH                                      (100 + SQL_CODE_MONTH)
#define SQL_INTERVAL_DAY                                        (100 + SQL_CODE_DAY)
#define SQL_INTERVAL_HOUR                                       (100 + SQL_CODE_HOUR)
#define SQL_INTERVAL_MINUTE                                     (100 + SQL_CODE_MINUTE)
#define SQL_INTERVAL_SECOND                     (100 + SQL_CODE_SECOND)
#define SQL_INTERVAL_YEAR_TO_MONTH                      (100 + SQL_CODE_YEAR_TO_MONTH)
#define SQL_INTERVAL_DAY_TO_HOUR                        (100 + SQL_CODE_DAY_TO_HOUR)
#define SQL_INTERVAL_DAY_TO_MINUTE                      (100 + SQL_CODE_DAY_TO_MINUTE)
#define SQL_INTERVAL_DAY_TO_SECOND                      (100 + SQL_CODE_DAY_TO_SECOND)
#define SQL_INTERVAL_HOUR_TO_MINUTE                     (100 + SQL_CODE_HOUR_TO_MINUTE)
#define SQL_INTERVAL_HOUR_TO_SECOND                     (100 + SQL_CODE_HOUR_TO_SECOND)
#define SQL_INTERVAL_MINUTE_TO_SECOND           (100 + SQL_CODE_MINUTE_TO_SECOND)

The value I got for a query was 106 (ColumnDescriptor::data_type).

Personally I think that missing enum variant is a bug (in odbc-ffi or in the driver if value is not part of ODBC spec). Bugs are best handled with panic. So if you could verify a value is represented by an enum before casting to it or otherwise panic with the missing value in the message that would be the most useful.

[pacman82]

I'll convert all enums which are used as output parameters into a newtype Integer + Constants.

[pacman82]

Since this would be a breaking change I'll combine it with another breaking change I had in mind: Using idiomatic Rust naming schemes (Aka Camel Case).

[pacman82]

what do you mean? to dismantle the SqlReturn enum and expose it's variants as constants?

Yes, exactly