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

Enable some numeric cast releated clippy lints and fix them in the code base #4170

Merged

Conversation

weiznich
Copy link
Member

  • clippy::cast_possible_wrap
  • clippy::cast_possible_truncation
  • clippy::cast_sign_loss

These lints can point to serious problems if you hit one of the edge cases in low level unsafe/byte shuffling code

This is a reaction to
https://media.defcon.org/DEF%20CON%2032/DEF%20CON%2032%20presentations/DEF%20CON%2032%20-%20Paul%20Gerste%20-%20SQL%20Injection%20Isn't%20Dead%20Smuggling%20Queries%20at%20the%20Protocol%20Level.pdf

It fixes several places that could be possibly exploited by specially crafted values.

@weiznich weiznich added the maybe backport Maybe backport this pr to the latest diesel release label Aug 15, 2024
@weiznich weiznich requested a review from a team August 15, 2024 09:28
@weiznich weiznich force-pushed the prevent_protocol_level_size_overflows branch 2 times, most recently from 88b9074 to a37b7e2 Compare August 15, 2024 12:03
@weiznich weiznich added the run-benchmarks Used to indicate that github actions should run our benchmark suite label Aug 16, 2024
Copy link

github-actions bot commented Aug 16, 2024

🐰Bencher

ReportFri, August 23, 2024 at 05:24:23 UTC
Projectdiesel
Branchprevent_protocol_level_size_overflows-fad6b6df
Testbedubuntu-latest-sqlite
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)2,829.40 (+0.94%)3,222.62 (87.80%)
bench_insert/diesel/10✅ (view plot)7,096.90 (+1.40%)8,554.33 (82.96%)
bench_insert/diesel/100✅ (view plot)45,406.00 (+1.66%)56,450.31 (80.44%)
bench_insert/diesel/25✅ (view plot)13,658.00 (+1.71%)17,083.75 (79.95%)
bench_insert/diesel/50✅ (view plot)24,358.00 (+1.97%)31,373.33 (77.64%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)405,300.00 (+0.42%)430,844.14 (94.07%)
bench_medium_complex_query/diesel/1✅ (view plot)3,379.00 (+0.75%)3,754.34 (90.00%)
bench_medium_complex_query/diesel/10✅ (view plot)5,445.20 (+0.56%)5,900.23 (92.29%)
bench_medium_complex_query/diesel/100✅ (view plot)28,422.00 (+0.68%)31,296.65 (90.81%)
bench_medium_complex_query/diesel/1000✅ (view plot)250,300.00 (+0.62%)273,163.13 (91.63%)
bench_medium_complex_query/diesel/10000✅ (view plot)2,474,800.00 (+1.54%)3,035,579.61 (81.53%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)4,725.00 (-0.20%)4,887.19 (96.68%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)6,781.20 (+0.02%)6,799.82 (99.73%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)29,934.00 (+0.45%)31,929.87 (93.75%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)253,410.00 (+1.13%)295,487.09 (85.76%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)2,476,300.00 (+1.38%)2,979,735.62 (83.10%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)18,457.00 (-0.21%)19,098.99 (96.64%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)21,002.00 (-0.23%)21,821.39 (96.25%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)48,304.00 (-0.07%)48,878.41 (98.82%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)310,490.00 (+0.54%)335,214.94 (92.62%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)2,911,300.00 (+0.92%)3,304,515.98 (88.10%)
bench_trivial_query/diesel/1✅ (view plot)894.43 (+1.53%)1,094.84 (81.70%)
bench_trivial_query/diesel/10✅ (view plot)2,821.60 (+0.12%)2,872.99 (98.21%)
bench_trivial_query/diesel/100✅ (view plot)23,107.00 (+0.30%)24,134.72 (95.74%)
bench_trivial_query/diesel/1000✅ (view plot)217,740.00 (+0.50%)233,900.58 (93.09%)
bench_trivial_query/diesel/10000✅ (view plot)2,150,200.00 (+0.52%)2,317,018.90 (92.80%)
bench_trivial_query/diesel_boxed/1✅ (view plot)1,483.10 (-0.60%)1,634.31 (90.75%)
bench_trivial_query/diesel_boxed/10✅ (view plot)3,410.10 (-0.63%)3,773.33 (90.37%)
bench_trivial_query/diesel_boxed/100✅ (view plot)23,877.00 (+0.66%)26,193.10 (91.16%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)221,240.00 (+0.97%)252,741.96 (87.54%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)2,158,000.00 (+0.66%)2,368,757.81 (91.10%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)5,520.00 (+1.07%)6,393.57 (86.34%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)7,997.60 (+0.31%)8,363.26 (95.63%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)31,862.00 (-0.47%)34,429.97 (92.54%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)263,240.00 (-0.44%)282,922.14 (93.04%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)2,521,700.00 (-1.13%)3,008,262.89 (83.83%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link

github-actions bot commented Aug 16, 2024

🐰Bencher

ReportFri, August 23, 2024 at 05:24:20 UTC
Projectdiesel
Branchprevent_protocol_level_size_overflows-fad6b6df
Testbedubuntu-latest-mysql
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)1,179,200.00 (+1.23%)1,392,191.99 (84.70%)
bench_insert/diesel/10✅ (view plot)1,292,100.00 (-3.17%)2,007,583.97 (64.36%)
bench_insert/diesel/100✅ (view plot)2,214,500.00 (-1.02%)2,600,540.35 (85.16%)
bench_insert/diesel/25✅ (view plot)1,716,600.00 (+0.75%)1,906,505.45 (90.04%)
bench_insert/diesel/50✅ (view plot)2,010,000.00 (+6.44%)3,822,666.11 (52.58%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)16,503,000.00 (+0.29%)17,210,490.88 (95.89%)
bench_medium_complex_query/diesel/1✅ (view plot)114,330.00 (+0.43%)121,553.85 (94.06%)
bench_medium_complex_query/diesel/10✅ (view plot)152,710.00 (+1.02%)175,573.13 (86.98%)
bench_medium_complex_query/diesel/100✅ (view plot)216,870.00 (-0.08%)219,911.02 (98.62%)
bench_medium_complex_query/diesel/1000✅ (view plot)855,380.00 (+0.29%)892,020.58 (95.89%)
bench_medium_complex_query/diesel/10000✅ (view plot)7,423,300.00 (+0.04%)7,465,749.45 (99.43%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)117,650.00 (+0.74%)130,459.31 (90.18%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)152,380.00 (-0.36%)161,672.00 (94.25%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)220,420.00 (-0.47%)237,990.33 (92.62%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)870,170.00 (+0.67%)956,186.00 (91.00%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)7,513,700.00 (+0.18%)7,714,776.36 (97.39%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)191,580.00 (-0.09%)194,367.60 (98.57%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)237,820.00 (+0.53%)256,438.18 (92.74%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)312,830.00 (+1.37%)375,982.87 (83.20%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)983,870.00 (+0.25%)1,019,691.38 (96.49%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)8,073,900.00 (-0.31%)8,503,866.17 (94.94%)
bench_trivial_query/diesel/1✅ (view plot)65,069.00 (-1.99%)87,378.25 (74.47%)
bench_trivial_query/diesel/10✅ (view plot)73,648.00 (+0.53%)79,382.40 (92.78%)
bench_trivial_query/diesel/100✅ (view plot)134,920.00 (+0.16%)138,196.80 (97.63%)
bench_trivial_query/diesel/1000✅ (view plot)687,150.00 (-0.36%)728,710.58 (94.30%)
bench_trivial_query/diesel/10000✅ (view plot)6,170,300.00 (+0.05%)6,217,962.54 (99.23%)
bench_trivial_query/diesel_boxed/1✅ (view plot)66,968.00 (-1.35%)82,451.85 (81.22%)
bench_trivial_query/diesel_boxed/10✅ (view plot)73,696.00 (+0.05%)74,254.55 (99.25%)
bench_trivial_query/diesel_boxed/100✅ (view plot)136,160.00 (+1.13%)158,725.24 (85.78%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)693,410.00 (-0.19%)715,288.44 (96.94%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)6,120,700.00 (-0.64%)6,790,568.70 (90.14%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)129,640.00 (+1.32%)154,886.25 (83.70%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)135,840.00 (+1.14%)158,703.13 (85.59%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)198,890.00 (+0.45%)212,295.09 (93.69%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)757,690.00 (+0.21%)781,372.33 (96.97%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)6,379,700.00 (+0.52%)6,873,454.16 (92.82%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link

github-actions bot commented Aug 16, 2024

🐰Bencher

ReportFri, August 23, 2024 at 05:24:23 UTC
Projectdiesel
Branchprevent_protocol_level_size_overflows-fad6b6df
Testbedubuntu-latest-postgres
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)209,180.00 (-1.07%)247,530.62 (84.51%)
bench_insert/diesel/10✅ (view plot)250,670.00 (-1.46%)313,264.29 (80.02%)
bench_insert/diesel/100✅ (view plot)588,740.00 (-0.02%)591,105.24 (99.60%)
bench_insert/diesel/25✅ (view plot)306,090.00 (-0.51%)332,614.44 (92.03%)
bench_insert/diesel/50✅ (view plot)415,850.00 (+1.78%)524,058.87 (79.35%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)6,168,000.00 (-0.03%)6,201,789.09 (99.46%)
bench_medium_complex_query/diesel/1✅ (view plot)83,016.00 (-0.25%)86,504.72 (95.97%)
bench_medium_complex_query/diesel/10✅ (view plot)93,963.00 (+0.50%)100,896.41 (93.13%)
bench_medium_complex_query/diesel/100✅ (view plot)120,770.00 (-0.51%)131,160.15 (92.08%)
bench_medium_complex_query/diesel/1000✅ (view plot)424,000.00 (-0.58%)465,729.53 (91.04%)
bench_medium_complex_query/diesel/10000✅ (view plot)3,398,400.00 (+0.97%)3,886,941.07 (87.43%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)86,461.00 (+0.11%)87,868.53 (98.40%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)93,327.00 (+0.74%)103,470.19 (90.20%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)124,150.00 (+0.47%)132,714.36 (93.55%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)436,250.00 (+1.26%)517,276.32 (84.34%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)3,366,800.00 (+0.55%)3,642,349.08 (92.43%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)240,870.00 (-0.61%)265,958.40 (90.57%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)242,940.00 (-1.12%)289,484.47 (83.92%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)283,880.00 (+0.30%)296,540.36 (95.73%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)612,310.00 (+0.56%)662,951.45 (92.36%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)3,755,300.00 (+1.59%)4,631,843.96 (81.08%)
bench_trivial_query/diesel/1✅ (view plot)66,388.00 (+0.64%)72,651.16 (91.38%)
bench_trivial_query/diesel/10✅ (view plot)68,142.00 (-0.07%)68,961.39 (98.81%)
bench_trivial_query/diesel/100✅ (view plot)102,970.00 (+2.04%)133,690.00 (77.02%)
bench_trivial_query/diesel/1000✅ (view plot)394,140.00 (+0.49%)422,514.11 (93.28%)
bench_trivial_query/diesel/10000✅ (view plot)3,510,200.00 (+1.30%)4,183,433.43 (83.91%)
bench_trivial_query/diesel_boxed/1✅ (view plot)68,541.00 (-0.24%)71,345.49 (96.07%)
bench_trivial_query/diesel_boxed/10✅ (view plot)71,658.00 (+0.93%)81,466.06 (87.96%)
bench_trivial_query/diesel_boxed/100✅ (view plot)103,910.00 (+2.56%)142,539.00 (72.90%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)393,320.00 (-1.92%)523,154.58 (75.18%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)3,521,900.00 (+1.30%)4,192,899.25 (84.00%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)152,010.00 (+0.45%)162,138.29 (93.75%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)153,310.00 (-0.63%)169,782.18 (90.30%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)192,710.00 (+0.87%)217,360.47 (88.66%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)497,620.00 (+1.18%)584,231.78 (85.18%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)3,745,600.00 (+1.23%)4,421,067.61 (84.72%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@weiznich
Copy link
Member Author

I double checked the sqlite benchmarks locally and this doesn't seem to have an impact on performance.

@Sytten
Copy link
Contributor

Sytten commented Aug 16, 2024

Also for reference, SQLx is also working on it.
launchbadge/sqlx#3440
They decided to go with a security advisory.

@weiznich
Copy link
Member Author

@diesel/core I would like to issue a patch release next Friday (23.8) with this change and a few others (marked as maybe-backport). Are there any objections?

Copy link
Member

@Ten0 Ten0 left a comment

Choose a reason for hiding this comment

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

That's really nice, thanks a lot for working on this!
I'd like to propose the following additional commit that explicits a few more types to guarantee match between the expects and the actual target types in case of change of signature of the underlying called functions.
(Arguably most of them are unlikely to change.)
(pushed it, feel free to revert)

Comment on lines +19 to +20
column_count: libc::c_int,
row_count: libc::c_int,
Copy link
Member

Choose a reason for hiding this comment

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

It looks weird to use this in the Rust universe especially if the external APIs are still using usize. How about just returning an error from new (or panicking) if the conversion fails?
(Ten0@7262cc5)

@weiznich
Copy link
Member Author

I'd like to propose the following additional commit that explicits a few more types to guarantee match between the expects and the actual target types in case of change of signature of the underlying called functions.
(Arguably most of them are unlikely to change.)
(pushed it, feel free to revert)

That's a good idea. Thanks for pushing this change.

weiznich and others added 3 commits August 23, 2024 06:56
…de base

* clippy::cast_possible_wrap
* clippy::cast_possible_truncation
* clippy::cast_sign_loss

These lints can point to serious problems if you hit one of the edge
cases in low level unsafe/byte shuffling code

This is a reaction to
https://media.defcon.org/DEF%20CON%2032/DEF%20CON%2032%20presentations/DEF%20CON%2032%20-%20Paul%20Gerste%20-%20SQL%20Injection%20Isn't%20Dead%20Smuggling%20Queries%20at%20the%20Protocol%20Level.pdf

It fixes several places that could be possibly exploited by specially
crafted values.
To avoid that a change in the types of the underlying called functions would result not in a compilation error but instead an incorrect `expect`.
@weiznich weiznich force-pushed the prevent_protocol_level_size_overflows branch from 62520cb to fad6b6d Compare August 23, 2024 04:58
@weiznich weiznich added this pull request to the merge queue Aug 23, 2024
Merged via the queue into diesel-rs:master with commit 9eccd7d Aug 23, 2024
50 checks passed
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 23, 2024
…l_size_overflows

Enable some numeric cast releated clippy lints and fix them in the code base
@weiznich weiznich mentioned this pull request Aug 23, 2024
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 23, 2024
…l_size_overflows

Enable some numeric cast releated clippy lints and fix them in the code base
weiznich added a commit to weiznich/diesel_async that referenced this pull request Aug 23, 2024
This is similar to diesel-rs/diesel#4170, it's
just not a serve as the diesel change as we do not found any critical
cast here. I also investigated the implementation in the postgres crate
and it seems to be fine as well (i.e error on too large buffer sizes
instead silently truncating)
@Sytten
Copy link
Contributor

Sytten commented Aug 24, 2024

@weiznich If you want to add a regression test here is what sqlx did launchbadge/sqlx@f9e5176

@weiznich
Copy link
Member Author

@Sytten A PR that adds such a test would be very welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maybe backport Maybe backport this pr to the latest diesel release run-benchmarks Used to indicate that github actions should run our benchmark suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants