Issue #4699: range end index 4 out of range for slice of length 0 still a problem

[rp-]

Backend

MySQL

Diesel version

2.3.2

Diesel Features

mysql

Operating System Version

ArchLinux mariadb-libs 12.0.2-1

Third party libraries

No response

What do you want to do?

As mentioned in #4699 I should open up a new discussion.
ArchLinux now recieved mariadb-libs 12.0.2, that should use mariadb connector-c 3.4.7 with the fix mentioned in #4699.

But the bug/error from #4699 is still occurring on my system, to reproduce you could still use the code from the sample:

https://github.com/BlackDex/diesel-issue-4699

I don't have a 3.4.7 docker image yet but maybe @BlackDex could build one to test or could also verify that the bug is still there with 3.4.7.

Compile time error

none, but this runtime error:

rp@frami /tmp/diesel-issue-4699 ±main⚡ » DATABASE_URL="mysql://example:example@127.0.0.1:3306/example" RUST_BACKTRACE=1 cargo run                                                                                                    130 ↵
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.47s
     Running `target/debug/dieseldebuglcrate`
mysql/mariadb lib version: 30407
mysql/mariadb srv version: 10.8.8-MariaDB
mysql/mariadb version id: 100808

thread 'main' panicked at src/main.rs:50:10:
Error loading users: DeserializationError(DeserializeFieldError { field_name: Some("client_kdf_memory"), error: "Received a buffer with an invalid size while trying to read a Long value: Expected at least 4 bytes but got 0" })
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs:697:5
   1: core::panicking::panic_fmt
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panicking.rs:75:14
   2: core::result::unwrap_failed
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/result.rs:1765:5
   3: core::result::Result<T,E>::expect
             at /home/rp/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:1119:23
   4: dieseldebuglcrate::main
             at ./src/main.rs:50:10
   5: core::ops::function::FnOnce::call_once
             at /home/rp/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:253:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

What code do you already have?

https://github.com/BlackDex/diesel-issue-4699

Additional details

No response

Checklist

• I have already looked over the issue tracker and the discussion forum for similar possible closed issues.

[weiznich]

ArchLinux now recieved mariadb-libs 12.0.2, that should use mariadb connector-c 3.4.7 with the fix mentioned in #4699.

Should is very vague here. Did you actually verify that the fix is included? That means did you verify that the code change is actually there in the version of libmariadb shipped by arch linux and not only that actual versions line up.

The next step would still be to build a reproducible example that includes the environment. This likely requires you to provide a docker image.

Finally given that the previous issue was an issue in the underlying C code: What steps did you take to ensure that this is an actual diesel issue and not again an upstream issues? The later shouldn't be reported here but in the relevant upstream issue tracker (so mariadb's issue tracker if it is a problem in their lib or the arch linux tracker if it's only related to the version they ship).

Also I want to point to the diesel 2.3 release announcements. As written there: Diesel needs a backend maintainer for the Mysql/MariaDB backend until that happens issues like this likely won't be addressed anymore due to capacity reasons.

[weiznich]

To be honest: That's not my problem as I don't use the MySQL backend at all. As written in the 2.3 release announcements, that backend really needs someone that cares about it otherwise we likely end in a situation where it will become deprecated with the next few releases, exactly because of such things.

Otherwise as for distros shipping it: Please reach out to your distro then. If they claim the package is compatible with libmysqlclient that's a bug there then as well as it is as demonstrated not compatible.

[weiznich]

Also to be explicit about the underlying problem here:

The documentation (both mysql and mariadb) states that lenght is ignored/not set for timestamps and numeric values as these are fixed size buffer. If that would be the case that wouldn't be a problem for us as we set the length beforehand.

diesel/diesel/src/mysql/connection/bind.rs

Lines 381 to 387 in a641d7c

	let len = known_buffer_size_for_ffi_type(tpe).unwrap_or(1);
	let (ptr, length, capacity) = bind_buffer(vec![0; len]);
	Self {
	tpe,
	bytes: ptr,
	length,

So the problem is that this documentation entry is incorrect as libmariadb overwrites the lenght we set with a zero value.

[rp-]

I see, thanks for stepping in.

[weiznich]

Also cc @grooverdan again, maybe they can help this time?

[simonzkl]

I don't think the mariadb client change is necessarily wrong. Unless I'm misunderstanding it, they made it so mysql_stmt_bind_result is now responsible for initialising MYSQL_BIND, where previously it was the responsibility of the caller.

However this definitely should have been considered a breaking change, as the initialisation responsibility was never explicitly documented. It's also different to what libmysqlclient does.

I feel like at this point libmariadb/libmysqlclient are ABI incompatible and package repositories should stop trying to package libmariadb as libmysqlclient-compat because they have a high risk of triggering subtle undefined behaviour.

I suppose what we'd need to change in diesel is to move the data length detection logic for fixed-size types to a later point, when we're extracting the value out. Though I'm unfamiliar with the code base and couldn't find an obvious way to do this.

I decided for now to just downgrade to libmariadb 3.3.15 which avoids this issue and the breaking TLS changes.

I feel for the maintainers having to deal with this though. Totally understand if you drop support given the churn upstream has been causing.