hyperium / h3

MIT License
595 stars 76 forks source link

usize is pointer size dependant #223

Closed alexanderkjall closed 9 months ago

alexanderkjall commented 9 months ago

The unit tests are currently not working on 32-bit platforms: https://qa.debian.org/excuses.php?package=rust-h3

result without this patch:

failures:

---- qpack::prefix_int::test::codec_5_bits stdout ----
thread 'qpack::prefix_int::test::codec_5_bits' panicked at 'assertion failed: `(left == right)`
  left: `[95, 224, 255, 255, 255, 15]`,
 right: `[95, 224, 255, 255, 255, 255, 255, 255, 255, 255, 1]`', h3/src/qpack/prefix_int.rs:99:9

---- qpack::prefix_int::test::codec_8_bits stdout ----
thread 'qpack::prefix_int::test::codec_8_bits' panicked at 'assertion failed: `(left == right)`
  left: `[255, 128, 254, 255, 255, 15]`,
 right: `[255, 128, 254, 255, 255, 255, 255, 255, 255, 255, 1]`', h3/src/qpack/prefix_int.rs:99:9

failures:
    qpack::prefix_int::test::codec_5_bits
    qpack::prefix_int::test::codec_8_bits

test result: FAILED. 220 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.41s
seanmonstar commented 9 months ago

Thanks for the fix! My initial feeling was, should this even be using usize? Does the spec say which size is always used?

alexanderkjall commented 9 months ago

I agree that it feels a bit strange with usize, but I don't have very much domain knowledge about this and thought that a good step forward was to document current behaviour.

I'll happily rework it or close it if there is a better way forward :)

seanmonstar commented 9 months ago

Alright, I found where it's mentioned in the RFC 9204 4.1.1:

QPACK implementations MUST be able to decode integers up to and including 62 bits long.

So, I think prefix_int should probably be changed to work with u64s, not usizes.

alexanderkjall commented 9 months ago

I started to look at it, and changing it to u64 have some effects through the codebase.

I'll make an attempt on resolving this and report back.

alexanderkjall commented 9 months ago

I made an attempt at changing the encode/decode functions to operate on u64 instead of usize.

I think there is a number of structs that should also be reviewed for what data types they are using, I looked at InsertCountIncrement and section 4.4.3 of the RFC seems to indicate that it should be an u8 rather than an usize.

But I lack both the domain knowledge and enough free time to gain that domain knowledge for being comfortable with the quality of this PR, please see it as inspiration or work in progress rather than a finished thing.

seanmonstar commented 9 months ago

That's fine, thanks for what you've done so far! (Looks like it needs a cargo fmt run)