format: checked chunk addresses on the parallel read path
Three `chunk_info.address as usize` casts behind the `parallel` feature survived the conversion, because check-32bit-casts.sh linted only default features plus plugin-filters. On a 32-bit target with rayon a chunk address past 4 GiB still wrapped onto another part of the file. They go through addr::to_usize now, and the lane index (h % n, always < n) through saturating_usize. The script now lints no default features, default features, and every optional feature but szip (wasm32; the set with zstd, which does not build for wasm32, on the host, where the lint reports the same casts). With the old parallel_read.rs/lane_partition.rs it fails listing the four casts; the old script passed them. CHANGELOG and the design note give the exact count (119) and what is not covered. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
This commit is contained in:
+10
-3
@@ -28,14 +28,21 @@
|
||||
resolves it in dense and compact groups.
|
||||
|
||||
### Checked address conversion (2026-09-26)
|
||||
- **No 64-bit file value is truncated on a 32-bit target.** Every
|
||||
`u64 as usize` cast in `clawhdf5-format` (115) is gone: file addresses,
|
||||
- **No 64-bit file value is truncated on a 32-bit target.** All 119
|
||||
truncating `u64 as usize` casts in `clawhdf5-format` that clippy's
|
||||
`cast_possible_truncation` reports, under every feature the crate is
|
||||
built with in CI except `szip` (115 with default features and
|
||||
`plugin-filters`, 4 more behind `parallel`), are gone: file addresses,
|
||||
lengths and counts go through `addr::to_usize`, which fails with
|
||||
`FormatError::Overflow` where the value does not fit (wasm32 and other
|
||||
32-bit targets; it used to wrap onto another part of the file), and
|
||||
in-memory counts through `addr::saturating_usize`. On 64-bit targets
|
||||
nothing changes. `scripts/check-32bit-casts.sh` (run by `ci-test.sh`)
|
||||
lints the wasm32 build and fails on any new truncating cast.
|
||||
lints the crate with no default features, with default features, and
|
||||
with every optional feature but `szip` (for wasm32; the set with `zstd`,
|
||||
which does not build for wasm32, for the host), and fails on any new
|
||||
truncating cast. The facade, `clawhdf5-io` and `clawhdf5-ann` are not
|
||||
covered.
|
||||
|
||||
### Chunked full reads (2026-09-26)
|
||||
- **Chunks are decoded straight into the output, into reused buffers.** A
|
||||
|
||||
@@ -112,7 +112,8 @@ pub fn partition(
|
||||
|
||||
for idx in 0..num_items {
|
||||
let h = fxhash_combine(seed, idx as u64);
|
||||
let lane = (h % num_lanes as u64) as usize;
|
||||
// Below `num_lanes`, so it fits.
|
||||
let lane = crate::addr::saturating_usize(h % num_lanes as u64);
|
||||
lanes[lane].push(idx);
|
||||
}
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@
|
||||
//! The lane assignment is seeded by dataset metadata so repeated reads of
|
||||
//! the same region produce identical partitions (cache-friendly, reproducible).
|
||||
|
||||
use crate::addr::to_usize;
|
||||
use crate::chunked_read::ChunkInfo;
|
||||
use crate::error::FormatError;
|
||||
use crate::filter_pipeline::FilterPipeline;
|
||||
@@ -210,7 +211,7 @@ pub fn decompress_chunks_lane_partitioned(
|
||||
|
||||
for &index in &indices {
|
||||
let chunk_info = &chunks[index];
|
||||
let c_addr = chunk_info.address as usize;
|
||||
let c_addr = to_usize(chunk_info.address)?;
|
||||
let size = chunk_info.chunk_size as usize;
|
||||
|
||||
if c_addr
|
||||
@@ -288,7 +289,7 @@ pub fn decompress_chunks_parallel(
|
||||
.par_iter()
|
||||
.enumerate()
|
||||
.map(|(index, chunk_info)| {
|
||||
let c_addr = chunk_info.address as usize;
|
||||
let c_addr = to_usize(chunk_info.address)?;
|
||||
let size = chunk_info.chunk_size as usize;
|
||||
if c_addr
|
||||
.checked_add(size)
|
||||
@@ -332,7 +333,7 @@ pub fn decompress_chunks_sequential(
|
||||
) -> Result<Vec<Vec<u8>>, FormatError> {
|
||||
let mut result = Vec::with_capacity(chunks.len());
|
||||
for chunk_info in chunks {
|
||||
let c_addr = chunk_info.address as usize;
|
||||
let c_addr = to_usize(chunk_info.address)?;
|
||||
let size = chunk_info.chunk_size as usize;
|
||||
if c_addr
|
||||
.checked_add(size)
|
||||
|
||||
@@ -383,10 +383,11 @@ fast path within benchmark noise.
|
||||
attribute names through the name indexes (`group_v2::resolve_child`,
|
||||
`attribute::find_attribute_in_file`; creation-order lookups by name do
|
||||
not exist in the API, so the creation-order index is still only listed),
|
||||
`addr::to_usize`/`saturating_usize` for all 115 `u64 as usize` casts in
|
||||
`clawhdf5-format` (the 133 above counted any `*addr*/*offset* as usize`,
|
||||
mostly widening `u8`/`u32` casts; `scripts/check-32bit-casts.sh` lints
|
||||
wasm32 for the truncating ones), and `Group::entries`/`File::group_at`.
|
||||
`addr::to_usize`/`saturating_usize` for all 119 truncating `u64 as usize`
|
||||
casts clippy finds in `clawhdf5-format` under any CI-built feature set but
|
||||
`szip` (the 133 above counted any `*addr*/*offset* as usize`, mostly
|
||||
widening `u8`/`u32` casts; `scripts/check-32bit-casts.sh` lints those
|
||||
feature sets for new ones), and `Group::entries`/`File::group_at`.
|
||||
The facade, io and ann casts are not converted.
|
||||
|
||||
**M1 — metadata over the trait, in-memory impl identical to today (2–3 weeks).**
|
||||
|
||||
@@ -5,8 +5,18 @@
|
||||
# of the file. File values go through `addr::to_usize` (a clean error) and
|
||||
# in-memory counts through `addr::saturating_usize`.
|
||||
#
|
||||
# Lints wasm32-unknown-unknown with clippy's cast_possible_truncation and
|
||||
# fails on any u64 -> usize finding (other truncations are not checked here).
|
||||
# Lints with clippy's cast_possible_truncation and fails on any u64 -> usize
|
||||
# finding (other truncations are not checked here), once per feature set
|
||||
# below. Together the sets compile every feature-gated line of the crate that
|
||||
# ci-test.sh builds: features only add code, except `not(feature = ...)`
|
||||
# paths for std/checksum/fast-checksum/szip, which the no-default-features
|
||||
# and default sets cover. szip is left out (it needs libaec), as in
|
||||
# ci-test.sh.
|
||||
#
|
||||
# The sets are linted for wasm32 where they build there. zstd links a C
|
||||
# library that does not build for wasm32, so the set with it is linted for
|
||||
# the host: the lint reports u64 -> usize casts whatever the target's
|
||||
# pointer width, and the crate has no pointer-width-dependent code.
|
||||
#
|
||||
# Usage:
|
||||
# ./scripts/check-32bit-casts.sh
|
||||
@@ -16,19 +26,42 @@
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
TARGET="wasm32-unknown-unknown"
|
||||
echo "==> Checking for truncating u64 -> usize casts in clawhdf5-format ($TARGET)"
|
||||
WASM="wasm32-unknown-unknown"
|
||||
ALL_BUT_ZSTD="parallel,lz4,pcodec,fast-checksum,blake3_hash,plugin-filters,lookup-stats"
|
||||
|
||||
out=$(cargo clippy -p clawhdf5-format --target "$TARGET" \
|
||||
--features plugin-filters --message-format short \
|
||||
-- -A clippy::all -W clippy::cast_possible_truncation 2>&1) || {
|
||||
echo "$out"
|
||||
echo "==> clippy failed" >&2
|
||||
exit 1
|
||||
}
|
||||
found=$(grep -F 'casting `u64` to `usize`' <<<"$out" || true)
|
||||
if [ -n "$found" ]; then
|
||||
echo "$found"
|
||||
# target|cargo feature arguments
|
||||
SETS=(
|
||||
"$WASM|--no-default-features"
|
||||
"$WASM|--no-default-features --features std,checksum"
|
||||
"$WASM|"
|
||||
"$WASM|--features $ALL_BUT_ZSTD"
|
||||
"host|--features $ALL_BUT_ZSTD,zstd"
|
||||
)
|
||||
|
||||
status=0
|
||||
for set in "${SETS[@]}"; do
|
||||
target=${set%%|*}
|
||||
args=${set#*|}
|
||||
target_args=()
|
||||
if [ "$target" != host ]; then
|
||||
target_args=(--target "$target")
|
||||
fi
|
||||
echo "==> Checking for truncating u64 -> usize casts in clawhdf5-format ($target: ${args:-default features})"
|
||||
# shellcheck disable=SC2086 # $args is a list of arguments
|
||||
out=$(cargo clippy -p clawhdf5-format "${target_args[@]}" $args \
|
||||
--message-format short \
|
||||
-- -A clippy::all -W clippy::cast_possible_truncation 2>&1) || {
|
||||
echo "$out"
|
||||
echo "==> clippy failed" >&2
|
||||
exit 1
|
||||
}
|
||||
found=$(grep -F 'casting `u64` to `usize`' <<<"$out" || true)
|
||||
if [ -n "$found" ]; then
|
||||
echo "$found"
|
||||
status=1
|
||||
fi
|
||||
done
|
||||
if [ "$status" -ne 0 ]; then
|
||||
echo "==> use addr::to_usize (file values) or addr::saturating_usize (in-memory counts)" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
Reference in New Issue
Block a user