Repository: diesel-rs/diesel
File: diesel/src/mysql/connection/bind.rs (Rust)

Code before:
            length,
            capacity,
            flags,
            is_null: super::raw::ffi_false(),
            is_truncated: Some(super::raw::ffi_false()),
        }
    }

    fn is_truncated(&self) -> bool {
        self.is_truncated.unwrap_or(super::raw::ffi_false()) != super::raw::ffi_false()
    }

    fn is_fixed_size_buffer(&self) -> bool {
        known_buffer_size_for_ffi_type(self.tpe).is_some()
    }

    pub(super) fn value(&'_ self) -> Option<MysqlValue<'_>> {
        if self.is_null() {
            None
        } else {
            let data = self.bytes?;
            let tpe = (self.tpe, self.flags).into();

            // FIXME: Could be unsafe. Need to make sure the buffer is large enough.
            let length = known_buffer_size_for_ffi_type(self.tpe)
                .unwrap_or(self.length.try_into().expect("Usize is at least 32 bit"));

            let slice = unsafe {
                // We know that this points to a slice and the pointer is not null at this
                // location
                // The length pointer is valid as long as none missuses `bind_for_truncated_data`
                // as this is the only location that updates the length field before the corresponding data are
                // written. At the time of writing this comment, the `BindData::bind_for_truncated_data`
                // function is only called by `Binds::populate_dynamic_buffers` which ensures the corresponding
                // invariant.
                std::slice::from_raw_parts(data.as_ptr(), length)
            };
            Some(MysqlValue::new_internal(slice, tpe))
        }
    }

    pub(super) fn is_null(&self) -> bool {
        self.is_null != ffi::my_bool::default()
    }

    fn update_buffer_length(&mut self) {
        use std::cmp::min;

        let actual_bytes_in_buffer = min(
            self.capacity,
            self.length.try_into().expect("Usize is at least 32 bit"),

Review comment (suggestion):
> The tricky bit is to reason about whether the buffer is sized that many bytes (which likely can be checked looking at the capacity field of the BindData struct) and if all possible byte patterns are valid for the target type.

I don't really understand where this would be an issue. The way I see it we already set the correct capacity for fixed-sized types when we first initialise `BindData`. MariaDB then resets the length, but the underlying buffer/capacity should stay untouched, no? And dynamically-sized types should already be handled as the breaking mariadb change only affected fixed-sized types.

Though I think it would be safer to set the correct `self.length` right after calling the mysql ffi functions. It's hard to tell which other functions in the module rely on `self.length` being correct.

Code after:
        Self {
            tpe,
            bytes: ptr,
            length,
            capacity,
            flags,
            is_null: super::raw::ffi_false(),
            is_truncated: Some(super::raw::ffi_false()),
        }
    }

    fn is_truncated(&self) -> bool {
        self.is_truncated.unwrap_or(super::raw::ffi_false()) != super::raw::ffi_false()
    }

    fn is_fixed_size_buffer(&self) -> bool {
        known_buffer_size_for_ffi_type(self.tpe).is_some()
    }

    pub(super) fn value(&'_ self) -> Option<MysqlValue<'_>> {
        if self.is_null() {
            None
        } else {
            let data = self.bytes?;
            let tpe = (self.tpe, self.flags).into();

            // Newer libmariadbclient versions overwrite the length field with zero values
            // in some cases (mostly when we load a zero value). As we don't reset the length value
            // and also reuse the underlying buffers for more than one row this is problematic
            // for diesel. To workaround that issue we instead reset the length value
            // for known statically sized types here.
            //
            // This is "safe" for all statically sized types as they are only "numeric" values
            // that consider every possible byte pattern as valid. This also includes timestamps,
            // but the MYSQL_TIME type consists only of numbers as well.
            //
            // To prevent a potential memory exposure we always initialize the buffer with
            // zeros, so the worst thing that could happen is that you get the value from a previous
            // row if the underlying library fails to reset the bytes
            //
            // We assert that we don't read more than capacity bytes below as that's
            // the most important invariant to uphold here
            let length = if let Some(length) = known_buffer_size_for_ffi_type(self.tpe) {
                debug_assert!(length <= self.length.try_into().expect("Usize is at least 32 bit"),
                    "Libmysqlclient reported a larger size for a fixed size buffer without setting the truncated flag. \n\
                     This is a bug somewhere. Please open an issue with reproduction steps at \
                     https://github.com/diesel-rs/diesel/issues/new \n\
                     Length: {length}, Capacity: {}, Type: {:?}", self.capacity, self.tpe
                );
                length
            } else {