Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

really test decode_varint_slow #977

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

mumbleskates
Copy link
Contributor

@mumbleskates mumbleskates commented Feb 2, 2024

The varint::check helper accepts its encoded argument mutably, which means its implementation is at risk of letting decode_varint deplete bytes from its encoded argument before decode_varint_slow is tested. This is the cause for the awkward code from 2021, including <&[u8]>::clone(&encoded). However, this test is expecting success, so the code was forced to work.

The varint_overflow test, however, expects failure. It was not noticed until now that decode_varint_slow(&mut u64_max_plus_one) on line 1687 always attempts to decode an empty slice &[] and fails to decode for that reason, rather than testing what happens whan it attempts to decode an over-max varint.

This changes all the buffers in question to not be declared mutably, and instead reborrows them to clone the slice.

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only change the test, so there is no bug in the production code, that is good to hear. You are improving the test because it doesn't actually test the correct thing.

src/encoding.rs Outdated
decode_varint(&mut u64_max_plus_one).expect_err("decoding u64::MAX + 1 succeeded");
decode_varint_slow(&mut u64_max_plus_one)
decode_varint(&mut &*u64_max_plus_one).expect_err("decoding u64::MAX + 1 succeeded");
decode_varint_slow(&mut &*u64_max_plus_one)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest improving code readability here. The &* syntax doesn't explain what is going on. I would do something like this to explicitly create a separated buffer. I think the extra allocations are not a problem in test code.

Suggested change
decode_varint_slow(&mut &*u64_max_plus_one)
let mut buffer = Bytes::copy_from_slice(u64_max_plus_one);
decode_varint_slow(&mut buffer).expect_err("slow decoding u64::MAX + 1 succeeded");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote &* here because it is spelled similarly nearby. The explicit spelling of this action is .as_slice(), which I'm happy to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that's not quite right. It's already a &[u8]; this is a reborrow, so i think it's <&[u8]>::clone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made every occurrence of &* reborrows explicit; if you would prefer I can scope that down but it seems like an improvement.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote &* here because it is spelled similarly nearby. The explicit spelling of this action is .as_slice(), which I'm happy to do.

Sorry, I was not aware that the other code already uses &* syntax. Your first commit does the correct thing: make the tests uniform.

I made every occurrence of &* reborrows explicit; if you would prefer I can scope that down but it seems like an improvement.

I think the new syntax (as_slice and such) don't convey the intent of the call. What the code is doing is creating a fresh Buf for each call to decode. So I am thinking of words like copy or clone. Maybe a new function like fn decouple(&[u8]) -> impl Buf or something like that. Do you have ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the data is in a Vec, it does not implement Buf. To get a Buf, we need to get it as a primitive &[u8] slice, which does implement the trait. (From the bytes documentation: "The simplest Buf is a &[u8].")

We can do this either by using the existence of the Deref<Target = [u8]> trait on the vec to re-borrow the value tersely with &*, or you can get it explicitly with Vec::as_slice. In my opinion the more verbose way conveys the intent of the call perfectly, and i believe the general confusion around all of this (including the extistence of the original bug) is evidence that it's needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What might make the most sense is to just use the Bytes::from_static and then clone that. But I don't think it matters much. As for &* vs as_slice as_slice is much better imo.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know there might be some style improvements but none of this affects public code so we can merge this now and do follow ups in other PRs. Thanks!

@LucioFranco LucioFranco merged commit 9353407 into tokio-rs:master Feb 28, 2024
12 checks passed
caspermeijn added a commit to caspermeijn/prost that referenced this pull request Apr 2, 2024
_PROST!_ is a [Protocol Buffers](https://developers.google.com/protocol-buffers/) implementation for the [Rust Language](https://www.rust-lang.org/). `prost` generates simple, idiomatic Rust code from `proto2` and `proto3` files.

This patch updates brings a few new features and fixes:

- Bump MSRV to 1.70 (minimum supported Rust version)
- Rename cargo feature `prost-derive` to `derive` (tokio-rs#992)
- Add @generated comment on top of generated files (tokio-rs#935)
- Optimize implementation of prost::Name when generated by prost-build (tokio-rs#956)

## Dependencies
- build(deps): Allow itertools 0.12 (tokio-rs#948)
- build(deps): Allow heck 0.5 (tokio-rs#1012)
- build(deps): Allow multimap 0.10 (tokio-rs#1013)

## Documentation
- Improve protoc not found error message (tokio-rs#937)
- build: Add development container config (tokio-rs#949)
- docs: Fixed README typos (tokio-rs#952 / tokio-rs#967 / tokio-rs#970)

## Internal
- chore: Fix minimal versions (tokio-rs#920)
- fix: fq_message_name should begin with one dot (tokio-rs#981)
- improve encode_varint performance by bounding its loop (tokio-rs#940)
- style: Remove duplicate function call (tokio-rs#989)
- test: Improve test decode_varint_slow (tokio-rs#977)
- chore: Add dep: prefix to feature dependencies (tokio-rs#919)
- Minor clippy lint fixes. (tokio-rs#1006)
- chore: Use taiki-e/install-action to setup cargo-machete (tokio-rs#909)
- chore: Remove which dependency. (tokio-rs#962)
- chore: Update to actions/checkout@v4 (tokio-rs#910)

```
$ cargo semver-checks --workspace
     Parsing prost v0.12.4 (current)
      Parsed [   4.348s] (current)
     Parsing prost v0.12.3 (baseline, cached)
      Parsed [   0.099s] (baseline)
    Checking prost v0.12.3 -> v0.12.4 (minor change)
     Checked [   0.029s] 61 checks; 61 passed, 6 unnecessary
    Finished [   4.481s] prost
     Parsing prost-build v0.12.4 (current)
      Parsed [   5.488s] (current)
     Parsing prost-build v0.12.3 (baseline)
      Parsed [   3.819s] (baseline)
    Checking prost-build v0.12.3 -> v0.12.4 (minor change)
     Checked [   0.005s] 61 checks; 61 passed, 6 unnecessary
    Finished [   9.319s] prost-build
     Parsing prost-types v0.12.4 (current)
      Parsed [   3.150s] (current)
     Parsing prost-types v0.12.3 (baseline)
      Parsed [   3.192s] (baseline)
    Checking prost-types v0.12.3 -> v0.12.4 (minor change)
     Checked [   0.075s] 61 checks; 61 passed, 6 unnecessary
    Finished [   6.434s] prost-types
```
caspermeijn added a commit to caspermeijn/prost that referenced this pull request Apr 5, 2024
_PROST!_ is a [Protocol Buffers](https://developers.google.com/protocol-buffers/) implementation for the [Rust Language](https://www.rust-lang.org/). `prost` generates simple, idiomatic Rust code from `proto2` and `proto3` files.

This patch update brings new features and fixes:

- Bump MSRV to 1.70 (minimum supported Rust version)
- Rename cargo feature `prost-derive` to `derive` (tokio-rs#992)
- Add @generated comment on top of generated files (tokio-rs#935)
- Optimize implementation of prost::Name when generated by prost-build (tokio-rs#956)

## Dependencies
- build(deps): Allow itertools 0.12 (tokio-rs#948)
- build(deps): Allow heck 0.5 (tokio-rs#1012)
- build(deps): Allow multimap 0.10 (tokio-rs#1013)

## Documentation
- Improve protoc not found error message (tokio-rs#937)
- build: Add development container config (tokio-rs#949)
- docs: Fixed README typos (tokio-rs#952 / tokio-rs#967 / tokio-rs#970)

## Internal
- chore: Fix minimal versions (tokio-rs#920)
- fix: fq_message_name should begin with one dot (tokio-rs#981)
- improve encode_varint performance by bounding its loop (tokio-rs#940)
- style: Remove duplicate function call (tokio-rs#989)
- test: Improve test decode_varint_slow (tokio-rs#977)
- chore: Add dep: prefix to feature dependencies (tokio-rs#919)
- Minor clippy lint fixes. (tokio-rs#1006)
- chore: Use taiki-e/install-action to setup cargo-machete (tokio-rs#909)
- chore: Remove which dependency. (tokio-rs#962)
- chore: Update to actions/checkout@v4 (tokio-rs#910)
caspermeijn added a commit to caspermeijn/prost that referenced this pull request Apr 5, 2024
_PROST!_ is a [Protocol Buffers](https://developers.google.com/protocol-buffers/) implementation for the [Rust Language](https://www.rust-lang.org/). `prost` generates simple, idiomatic Rust code from `proto2` and `proto3` files.

This patch update brings new features and fixes:

- Bump MSRV to 1.70 (minimum supported Rust version)
- Rename cargo feature `prost-derive` to `derive` (tokio-rs#992)
- Add @generated comment on top of generated files (tokio-rs#935)
- Optimize implementation of prost::Name when generated by prost-build (tokio-rs#956)

## Dependencies
- build(deps): Allow itertools 0.12 (tokio-rs#948)
- build(deps): Allow heck 0.5 (tokio-rs#1012)
- build(deps): Allow multimap 0.10 (tokio-rs#1013)

## Documentation
- Improve protoc not found error message (tokio-rs#937)
- build: Add development container config (tokio-rs#949)
- docs: Fixed README typos (tokio-rs#952 / tokio-rs#967 / tokio-rs#970)

## Internal
- chore: Fix minimal versions (tokio-rs#920)
- fix: fq_message_name should begin with one dot (tokio-rs#981)
- improve encode_varint performance by bounding its loop (tokio-rs#940)
- style: Remove duplicate function call (tokio-rs#989)
- test: Improve test decode_varint_slow (tokio-rs#977)
- chore: Add dep: prefix to feature dependencies (tokio-rs#919)
- Minor clippy lint fixes. (tokio-rs#1006)
- chore: Use taiki-e/install-action to setup cargo-machete (tokio-rs#909)
- chore: Remove which dependency. (tokio-rs#962)
- chore: Update to actions/checkout@v4 (tokio-rs#910)
github-merge-queue bot pushed a commit that referenced this pull request Apr 5, 2024
_PROST!_ is a [Protocol Buffers](https://developers.google.com/protocol-buffers/) implementation for the [Rust Language](https://www.rust-lang.org/). `prost` generates simple, idiomatic Rust code from `proto2` and `proto3` files.

This patch update brings new features and fixes:

- Bump MSRV to 1.70 (minimum supported Rust version)
- Rename cargo feature `prost-derive` to `derive` (#992)
- Add @generated comment on top of generated files (#935)
- Optimize implementation of prost::Name when generated by prost-build (#956)

## Dependencies
- build(deps): Allow itertools 0.12 (#948)
- build(deps): Allow heck 0.5 (#1012)
- build(deps): Allow multimap 0.10 (#1013)

## Documentation
- Improve protoc not found error message (#937)
- build: Add development container config (#949)
- docs: Fixed README typos (#952 / #967 / #970)

## Internal
- chore: Fix minimal versions (#920)
- fix: fq_message_name should begin with one dot (#981)
- improve encode_varint performance by bounding its loop (#940)
- style: Remove duplicate function call (#989)
- test: Improve test decode_varint_slow (#977)
- chore: Add dep: prefix to feature dependencies (#919)
- Minor clippy lint fixes. (#1006)
- chore: Use taiki-e/install-action to setup cargo-machete (#909)
- chore: Remove which dependency. (#962)
- chore: Update to actions/checkout@v4 (#910)
gibbz00 pushed a commit to gibbz00/prost that referenced this pull request Apr 6, 2024
_PROST!_ is a [Protocol Buffers](https://developers.google.com/protocol-buffers/) implementation for the [Rust Language](https://www.rust-lang.org/). `prost` generates simple, idiomatic Rust code from `proto2` and `proto3` files.

This patch update brings new features and fixes:

- Bump MSRV to 1.70 (minimum supported Rust version)
- Rename cargo feature `prost-derive` to `derive` (tokio-rs#992)
- Add @generated comment on top of generated files (tokio-rs#935)
- Optimize implementation of prost::Name when generated by prost-build (tokio-rs#956)

## Dependencies
- build(deps): Allow itertools 0.12 (tokio-rs#948)
- build(deps): Allow heck 0.5 (tokio-rs#1012)
- build(deps): Allow multimap 0.10 (tokio-rs#1013)

## Documentation
- Improve protoc not found error message (tokio-rs#937)
- build: Add development container config (tokio-rs#949)
- docs: Fixed README typos (tokio-rs#952 / tokio-rs#967 / tokio-rs#970)

## Internal
- chore: Fix minimal versions (tokio-rs#920)
- fix: fq_message_name should begin with one dot (tokio-rs#981)
- improve encode_varint performance by bounding its loop (tokio-rs#940)
- style: Remove duplicate function call (tokio-rs#989)
- test: Improve test decode_varint_slow (tokio-rs#977)
- chore: Add dep: prefix to feature dependencies (tokio-rs#919)
- Minor clippy lint fixes. (tokio-rs#1006)
- chore: Use taiki-e/install-action to setup cargo-machete (tokio-rs#909)
- chore: Remove which dependency. (tokio-rs#962)
- chore: Update to actions/checkout@v4 (tokio-rs#910)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants