Panic triggered by Thrift's usage of library, introduced in 1.1.0

[nikclayton-dfinity]

I've reported what I think is a problem in Thrift's use of this library, with a reproduction recipe, in https://issues.apache.org/jira/browse/THRIFT-5131.

Just in case it's actually a bug in this library I'm reporting it here too. I did "git bisect" to find the commit that introduced the problem, it's f7d55af.

[dermesser]

Thank you for reporting! I committed the above to see if this may fix the thrift issue. It's essentially an off-by-one error that is neutral for all integers smaller than the default buffer (10 bytes), but panics due to out-of-bounds access for bigger integers (this is also why the provided tests pass for either version). I believe that 10 bytes is the maximum legal/sensible size for a varint, which is why it was implemented like this.

Would you mind trying to reproduce the issue on branch thrift_decode_test?

[nikclayton-dfinity]

This appears to fix the problem, thanks.

To be specific, when the dependencies section in Cargo.toml looks like:

[dependencies]
thrift = "0.13.0"
integer-encoding = "=1.1.3"

I see the panic, when I extend that to

[dependencies]
thrift = "0.13.0"
integer-encoding = "=1.1.3"

[patch.crates-io]
integer-encoding = { git = "https://github.com/dermesser/integer-encoding-rs", branch = "thrift_decode_test" }

it works. I'll update the issue I reported with Thrift.

[dermesser]

I'm glad I could help! I published version 1.1.4.

By the way, I think we worked in the same office in Zurich a few years ago. I seem to remember your name :-)

[nikclayton-dfinity]

Ah, yeah, that was me.

In other news, I've submitted a PR to Thrift to point to 1.1.4 or above of the library, however it's failing because of the use of await, see https://travis-ci.org/apache/thrift/jobs/658632104?utm_medium=notification&utm_source=github_status. Is this something you can address as well?

[Jens-G]

https://issues.apache.org/jira/browse/THRIFT-5111 still unsolved

[dermesser]

In theory, this should not happen: Without any async feature (tokio_async, futures_async) enabled, cargo shouldn't run or see the tests using await (which fail to compile here). They are, like the rest of the async code, guarded by the features I expect you not to have enabled.

Maybe this is related to an old compiler, which knows the keyword but cannot process it? I don't know the details on the rust compiler, at which stage the conditional compilation is evaluated, for example.

[dermesser]

This seems to have been fixed, judging from the jira ticket.