Introduce Safe Arithmetic - #8171
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a checked-arithmetic header and review guidance, plus comprehensive applications of ChangesChecked Arithmetic System for Host-Side Overflow Prevention
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cpp/src/tsne/barnes_hut_tsne.cuh (1)
60-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
FOUR_NNODESandFOUR_Nstill use unchecked host-side multiplication.Lines 89-90 compute
4 * nnodesand4 * ninvalue_idx. These can overflow even whennnodesitself is in range, which can corrupt kernel bounds.Suggested fix
- const value_idx FOUR_NNODES = 4 * nnodes; - const value_idx FOUR_N = 4 * n; + std::size_t const four_nnodes_sz = checked_mul<std::size_t>(nnodes_sz, 4); + std::size_t const four_n_sz = checked_mul<std::size_t>(static_cast<std::size_t>(n), 4); + const value_idx FOUR_NNODES = ML::narrow_cast<value_idx>(four_nnodes_sz); + const value_idx FOUR_N = ML::narrow_cast<value_idx>(four_n_sz);As per coding guidelines: host-side size/launch arithmetic using sub-
size_tintegers must use checked helpers (e.g.,ML::checked_mul<size_t>(...)) before narrowing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/tsne/barnes_hut_tsne.cuh` around lines 60 - 109, FOUR_NNODES and FOUR_N are computed with host-side unchecked multiplication in value_idx and can overflow; change their computation to do size_t checked multiplication (use checked_mul<std::size_t>(nnodes_sz, 4) and checked_mul<std::size_t>(static_cast<std::size_t>(n), 4) or similar), verify the resulting size_t fits into value_idx (compare to std::numeric_limits<value_idx>::max()), then narrow/cast the checked result to value_idx for FOUR_NNODES and FOUR_N; update any uses of FOUR_NNODES/FOUR_N to the new safe variables.cpp/src/explainer/kernel_shap.cu (1)
181-194:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate
len_sampleslower bound before computingnblks.Line 181 uses
nrows_X - len_samplesbeforelen_samplesis validated. A negativelen_samplesincreasesnblksand can launchexact_rows_kernelpast valid rows.Suggested fix
- nthreads = min(512, ncols); - nblks = nrows_X - len_samples; + if (len_samples < 0 || len_samples > nrows_X) { + RAFT_FAIL("kernel_dataset: len_samples (%d) must be in [0, %d]", len_samples, nrows_X); + } + + nthreads = min(512, ncols); + nblks = checked_sub<IdxT>(nrows_X, static_cast<IdxT>(len_samples)); @@ - if (len_samples > nrows_X) { - RAFT_FAIL("kernel_dataset: len_samples (%d) must be <= nrows_X (%d)", len_samples, nrows_X); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/explainer/kernel_shap.cu` around lines 181 - 194, Validate len_samples before computing nblks: ensure len_samples is checked for a valid non-negative (and <= nrows_X) value before you compute nblks = nrows_X - len_samples and before launching exact_rows_kernel; move the existing bounds check (the RAFT_FAIL that compares len_samples and nrows_X) to precede the nblks calculation/launch (or add an explicit check for len_samples < 0), so that nblks, exact_rows_kernel, and any subsequent logic use a validated len_samples value.cpp/src/svm/svc_impl.cuh (1)
248-250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnchecked products remain in batch-sizing and transform-count calculations.
Line 248-250 and Line 279 still use raw multiplication for size/count math (
n_batch * n_support * sizeof(math_t)andbatch_size * n_support). These should use checked helpers as well to avoid overflow in control-flow and iterator range calculations.Suggested fix
- if ((size_t)n_batch * model.n_support * sizeof(math_t) > buffer_size) { - n_batch = buffer_size / (model.n_support * sizeof(math_t)); + auto const kernel_bytes = + checked_mul<std::size_t>(static_cast<std::size_t>(n_batch), + static_cast<std::size_t>(model.n_support), + sizeof(math_t)); + if (kernel_bytes > static_cast<std::size_t>(buffer_size)) { + auto const denom = + checked_mul<std::size_t>(static_cast<std::size_t>(model.n_support), sizeof(math_t)); + n_batch = narrow_cast<int>(checked_div<std::size_t>(static_cast<std::size_t>(buffer_size), denom)); if (n_batch < 1) n_batch = 1; } @@ - int n_elems = batch_size * n_support; + int n_elems = narrow_cast<int>(checked_mul<std::size_t>(batch_size, n_support));As per coding guidelines, host-side size arithmetic passed to allocations/launch-related paths must use checked helpers instead of unchecked
int/mixed-width products.Also applies to: 279-279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/svm/svc_impl.cuh` around lines 248 - 250, The batch-sizing and transform-count checks use unchecked mixed-width products (n_batch * model.n_support * sizeof(math_t) and batch_size * model.n_support); replace those raw multiplications with the project's checked-size helpers (e.g., safe_mul/safe_mult or the existing checked helpers used elsewhere) so comparisons against buffer_size and assignments to n_batch use overflow-checked size_t arithmetic, and use the same checked helpers when computing transform counts for batch_size * model.n_support to ensure no overflow before converting to iterators/launch sizes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cuml/common/checked_arithmetic.hpp`:
- Around line 82-90: The magnitude check in widen_or_fail uses -static_cast<long
long>(value)/static_cast<long long>(value) which can invoke UB for extreme
signed/unsigned inputs; change the computation of abs_val to work entirely in
the unsigned/common type instead of long long: determine common =
std::common_type_t<std::make_unsigned_t<U>, std::make_unsigned_t<T>> then
compute abs_val by casting value to that unsigned/common type (for negative
values compute abs as static_cast<common>(0) - static_cast<common>(value) or
equivalent) so no long long intermediate is used; keep the subsequent comparison
to static_cast<common>(std::numeric_limits<T>::max()) and the RAFT_FAIL call
unchanged.
In `@cpp/include/cuml/tsa/arima_common.h`:
- Around line 74-90: The deallocation code must mirror the hardened size
arithmetic used during allocation to avoid integer overflow; update the
rmm_alloc.deallocate calls for mu, beta, ar, ma, sar, sma, and sigma2 to use
checked_mul<std::size_t> (and the same bs and order.* operands) and pass the
resulting std::size_t byte counts to rmm_alloc.deallocate just as allocate does,
ensuring the same casts/size computations are used for all pointers (mu, beta,
ar, ma, sar, sma, sigma2) and leaving rmm_alloc and checked_mul usage consistent
with the allocation block.
In `@cpp/src_prims/linalg/batched/matrix.cuh`:
- Line 1295: Change the declaration of batch_size to avoid silent narrowing from
std::size_t: wrap the call to A.batches() with ML::narrow_cast<int>(...) instead
of assigning directly to an int, i.e., use ML::narrow_cast to perform the
explicit checked narrowing for the variable batch_size and add the necessary ML
header/import if not already included.
- Line 1919: The pivot buffer P is undersized: allocate it with n2 * batch_size
elements (where n2 = n * n) instead of n * batch_size because
_direct_lyapunov_helper performs LU on the Kronecker-product matrix of size
n2×n2 and cublasgetrfBatched writes n2 pivots per batch; update the allocation
of rmm::device_uvector<int> P(...) to use ML::checked_mul<std::size_t>(n2,
batch_size) (or equivalent multiplication of n * n * batch_size) so P has n2 *
batch_size capacity before calling cublasgetrfBatched.
In `@cpp/src_prims/random/make_arima.cuh`:
- Around line 223-226: The shared_mem_size calculation in make_arima.cuh uses a
raw "* sizeof(double)" after doing checked_add/checked_mul, which bypasses
overflow checks; change the final multiplication to use
ML::checked_mul<std::size_t>(..., static_cast<std::size_t>(sizeof(double))) (or
wrap the entire previous checked result in ML::checked_mul with sizeof(double))
so the allocation size is computed with checked arithmetic (refer to the
variable shared_mem_size and the ML::checked_mul/checked_add calls in that
expression).
In `@cpp/src_prims/timeSeries/fillna.cuh`:
- Around line 112-115: You computed total with ML::checked_mul but later
kernels/allocations still use raw batch_size * n_obs; replace every raw
multiplication (batch_size * n_obs) used as extents/allocs/launch sizes with the
checked total variable (e.g., use total for the sizes passed to
rmm::device_uvector allocations, inclusive/exclusive scan extents, and any
kernel launch/grid/thread-count arguments) and ensure any other temporary
variables or calls that previously recomputed batch_size * n_obs now take total
(affecting indices_fwd, indices_bwd, and any calls that use
transform_op_fwd/transform_op_bwd extents) so all host-side size arithmetic is
the checked value.
In `@cpp/src_prims/timeSeries/stationarity.cuh`:
- Around line 313-318: Compute d_sD using checked/ widened integer arithmetic
before comparing to n_obs: cast d, s, and D to the unsigned/index type IdxT (or
a wider integer like int64_t) and perform a checked multiply for s*D (verify D
== 0 or product/s == D or check s <= max/ D) and a checked addition for d +
(s*D); if either operation would overflow, RAFT_FAIL with a clear message; only
after successful checked arithmetic assign d_sD and use it in the existing
bounds check and to compute n_obs_diff. Ensure you reference the symbols d, s,
D, d_sD, n_obs, RAFT_FAIL and n_obs_diff when making the changes.
In `@cpp/src/arima/batched_arima.cu`:
- Around line 131-132: Replace the raw multiplication used when resizing
fc_buffer with the checked arithmetic helper to avoid overflow: change the
fc_buffer.resize call that currently uses num_steps * batch_size to use
checked_mul<std::size_t>(num_steps, batch_size) (analogous to the existing
exog_fut_buffer.resize call which uses checked_mul<std::size_t>(num_steps,
order.n_exog, batch_size)); ensure the same stream argument is preserved.
- Around line 979-984: Compute d + s*D using the checked arithmetic helpers
instead of raw int ops to prevent overflow: replace the raw int computation of
d_sD with a checked multiplication of order.s and order.D and then a checked
addition with order.d (e.g., use checked_mul<int>(order.s, order.D) and
checked_add<int>(order.d, ...)), then use that checked d_sD when comparing to
n_obs and when calling checked_sub to produce diff_rows so all shape/allocation
math is done with overflow-checked values (referencing d_sD, order.d, order.s,
order.D, checked_mul, checked_add, n_obs, checked_sub, diff_rows).
In `@cpp/src/holtwinters/internal/hw_decompose.cuh`:
- Around line 260-269: The code currently computes end as raw start_periods *
frequency and computes batch_trend_n as unchecked batch_size * trend_len; change
these to use the checked multiplication/subtraction helpers and fail cleanly on
overflow: compute end via ML::checked_mul<int>(start_periods, frequency) (or
equivalent checked type) and validate it before using RAFT_FAIL, compute
trend_len using a checked subtraction from end and filter_size, and compute
batch_trend_n with ML::checked_mul<std::size_t>(batch_size, trend_len); keep
using RAFT_FAIL to report overflow errors if the checked helpers indicate
overflow.
In `@cpp/src/solver/lars_impl.cuh`:
- Around line 1114-1117: The kernel launch computes the grid size with int
arithmetic (n_active * ld_X) which can overflow; change the multiplication to
use widened/checked arithmetic like checked_mul<std::size_t>(n_active, ld_X)
(same as used for active_count) and compute the ceildiv on that size_t product
(use raft::ceildiv with size_t) then validate/cast the resulting grid size to
the int TPB-compatible type before launching get_vecs<<<...>>>; update the
launch expression that currently uses raft::ceildiv(n_active * ld_X, TPB) to use
the checked/widened product and checked ceil division to avoid overflow when
computing the grid dimension for get_vecs.
In `@cpp/src/tsa/auto_arima.cuh`:
- Around line 208-213: cumul_count is the checked multiplication result used for
allocation but the subsequent scan/for_each uses an unchecked product
(batch_size * n_sub) which may overflow; update the scan/for_each extent to use
the checked cumul_count (and sizeof where appropriate) and ensure any other
host-side size/index computations in this block use
ML::checked_mul<std::size_t>(batch_size, n_sub) (i.e., replace direct uses of
batch_size * n_sub with cumul_count or the checked multiplication) so d_cumul
and the scan bounds are consistent and safe.
---
Outside diff comments:
In `@cpp/src/explainer/kernel_shap.cu`:
- Around line 181-194: Validate len_samples before computing nblks: ensure
len_samples is checked for a valid non-negative (and <= nrows_X) value before
you compute nblks = nrows_X - len_samples and before launching
exact_rows_kernel; move the existing bounds check (the RAFT_FAIL that compares
len_samples and nrows_X) to precede the nblks calculation/launch (or add an
explicit check for len_samples < 0), so that nblks, exact_rows_kernel, and any
subsequent logic use a validated len_samples value.
In `@cpp/src/svm/svc_impl.cuh`:
- Around line 248-250: The batch-sizing and transform-count checks use unchecked
mixed-width products (n_batch * model.n_support * sizeof(math_t) and batch_size
* model.n_support); replace those raw multiplications with the project's
checked-size helpers (e.g., safe_mul/safe_mult or the existing checked helpers
used elsewhere) so comparisons against buffer_size and assignments to n_batch
use overflow-checked size_t arithmetic, and use the same checked helpers when
computing transform counts for batch_size * model.n_support to ensure no
overflow before converting to iterators/launch sizes.
In `@cpp/src/tsne/barnes_hut_tsne.cuh`:
- Around line 60-109: FOUR_NNODES and FOUR_N are computed with host-side
unchecked multiplication in value_idx and can overflow; change their computation
to do size_t checked multiplication (use checked_mul<std::size_t>(nnodes_sz, 4)
and checked_mul<std::size_t>(static_cast<std::size_t>(n), 4) or similar), verify
the resulting size_t fits into value_idx (compare to
std::numeric_limits<value_idx>::max()), then narrow/cast the checked result to
value_idx for FOUR_NNODES and FOUR_N; update any uses of FOUR_NNODES/FOUR_N to
the new safe variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c45394bc-58e1-4586-9939-9ed135ac614b
📒 Files selected for processing (36)
.coderabbit.yamlcpp/agents.mdcpp/include/cuml/common/checked_arithmetic.hppcpp/include/cuml/tsa/arima_common.hcpp/src/arima/batched_arima.cucpp/src/decisiontree/batched-levelalgo/quantiles.cuhcpp/src/explainer/kernel_shap.cucpp/src/explainer/tree_shap.cucpp/src/genetic/program.cucpp/src/glm/qn/qn.cuhcpp/src/hdbscan/detail/condense.cuhcpp/src/hdbscan/detail/predict.cuhcpp/src/hdbscan/detail/soft_clustering.cuhcpp/src/hdbscan/detail/utils.hcpp/src/hdbscan/runner.hcpp/src/holtwinters/internal/hw_decompose.cuhcpp/src/holtwinters/internal/hw_optim.cuhcpp/src/holtwinters/runner.cuhcpp/src/knn/knn.cucpp/src/randomforest/randomforest.cucpp/src/solver/lars_impl.cuhcpp/src/solver/sgd.cuhcpp/src/svm/linear.cucpp/src/svm/results.cuhcpp/src/svm/sparse_util.cuhcpp/src/svm/svc_impl.cuhcpp/src/tsa/auto_arima.cuhcpp/src/tsne/barnes_hut_tsne.cuhcpp/src/tsne/exact_tsne.cuhcpp/src/tsne/tsne_runner.cuhcpp/src/umap/simpl_set_embed/algo.cuhcpp/src/umap/simpl_set_embed/optimize_batch_kernel.cuhcpp/src_prims/linalg/batched/matrix.cuhcpp/src_prims/random/make_arima.cuhcpp/src_prims/timeSeries/fillna.cuhcpp/src_prims/timeSeries/stationarity.cuh
Replaces the custom representability check in `widen_or_fail` with `std::in_range`, and adds focused coverage for signed minima, out-of-range operands, arithmetic overflow, unsigned underflow, and invalid division.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
csadorf
left a comment
There was a problem hiding this comment.
Approving this with the expectation that this is a temporary band-aid, and that we work towards enabling the appropriate compiler warnings after all necessary cleanups.
dantegd
left a comment
There was a problem hiding this comment.
The change makes sense to me, I think the only thing missing is that yhe ARIMA backing-buffer sizing path still has unchecked int products feeding offsets and the final allocation size, so the PR doesn’t make ARIMA safe end-to-end yet. Examples in cpp/include/cuml/tsa/arima_common.h: append_buffer(T_dense, rd * rd * batch_size);
We could convert append_buffer/buf_offsets to take checked element counts and also check the internal n_elem * sizeof(ValType) + ALIGN - 1 arithmetic, though I'd treat that as a follow up and not blocker for the PR.
This comment has been minimized.
This comment has been minimized.
|
/merge |
@coderabbitai Please create a follow-up issue for this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="rapidsai/cuml"
pr_number="8171"
requester="csadorf"
# Try to locate the relevant PR comment URL for backlinking.
comment_url="$(
gh api "repos/${repo}/issues/${pr_number}/comments" --paginate \
--jq '.[] | select((.user.login == "csadorf") and (.body | contains("ARIMA backing-buffer sizing path"))) | .html_url' \
| tail -n 1
)"
if [[ -z "${comment_url}" ]]; then
comment_url="https://github.com/${repo}/pull/${pr_number}"
fi
title="Follow up: harden ARIMA backing-buffer sizing with checked arithmetic"
body="$(cat <<EOF
Requested by @${requester} as a follow-up from PR #${pr_number}.
## Summary
PR #${pr_number} introduces checked host-side arithmetic across many allocation, launch, span, and narrowing paths. One remaining ARIMA path still has unchecked integer products feeding backing-buffer offsets and final allocation sizing in:
- \`cpp/include/cuml/tsa/arima_common.h\`
Example pattern:
- \`append_buffer(T_dense, rd * rd * batch_size);\`
The follow-up should harden the ARIMA backing-buffer sizing path end-to-end.
## Rationale
Unchecked \`int\` products can overflow before being widened, which can produce incorrect buffer offsets or undersized final allocations. This undermines the safe-arithmetic goal for large-batch ARIMA workloads.
## Proposed work
- Convert \`append_buffer\` / \`buf_offsets\` sizing paths to take checked element counts.
- Replace unchecked products such as \`rd * rd * batch_size\` with checked arithmetic using the new helpers from \`cpp/include/cuml/common/checked_arithmetic.hpp\`.
- Check internal byte-size/alignment arithmetic, including \`n_elem * sizeof(ValType) + ALIGN - 1\`, for overflow.
- Preserve current behavior for valid workloads while failing deterministically with a clear error on overflow.
## Acceptance criteria
- ARIMA backing-buffer element counts are computed with checked arithmetic before being used for offsets or allocation sizes.
- Byte-size and alignment calculations inside the buffer sizing helper are overflow-checked.
- Existing ARIMA tests continue to pass.
- Add or update targeted tests if practical to cover overflow/failure behavior for the sizing helper.
## Backlinks
- PR: https://github.com/${repo}/pull/${pr_number}
- Discussion comment: ${comment_url}
EOF
)"
issue_url="$(gh issue create \
--repo "${repo}" \
--title "${title}" \
--body "${body}" \
--assignee "${requester}")"
printf '%s\n' "${issue_url}"Length of output: 2653
|
Safe host-side integer arithmetic in cuML
Motivation
A class of host-side bugs, including silent integer overflow, underflow, divide-by-zero, and truncating narrowing conversions, can produce undersized allocations, OOM crashes, and invalid kernel launch configurations on large-batch ML workloads.
The same root cause shows up in two directions:
n * batch_sizeis computed inintand then widened tosize_tfor the allocator. The widening does not undo the overflow that already happened.size_trow count, container size, or matrix dimension is assigned to anint, and the high bits are discarded. The narrowed value then propagates as a count or index into downstream code.Kernel-side arithmetic is intentionally out of scope. All checks added here run on the host at allocation, launch, and cast sites.
The idea: fix arithmetic at the source
Rather than redesign the affected APIs, this PR adds host-side safeguards at the points where the dangerous operation happens, so silent corruption becomes a clear
RAFT_FAILwith a message naming the offending operand.Two complementary mechanisms cover the two directions:
1. Widening is now safe end-to-end
Host-side arithmetic that feeds an allocation size, CUDA launch dimension, span constructor, or host pointer offset is now performed in the wider target type (
size_t/int64_t) and verified to fit that type.This is exposed via four helpers:
ML::checked_mul<T>(a, b, ...)RAFT_FAILon overflow.ML::checked_add<T>(a, b, ...)RAFT_FAILon overflow.ML::checked_sub<T>(a, b)RAFT_FAILon underflow.ML::checked_div<T>(a, b)RAFT_FAILon divide-by-zero and signed division overflow.2. Narrowing is now trapped, not eliminated
Many existing APIs require an
intparameter even when the natural type of the value issize_t. Restructuring those APIs is a larger effort and is out of scope here.Instead, this PR introduces:
ML::narrow_cast<T>(value)narrow_castis a runtime safety net, not an API redesign. The narrowing still happens at the call site, but it is now checked.Helper implementation notes
All helpers live in
cpp/include/cuml/common/checked_arithmetic.hppunder namespaceML. They are C++20 concept-constrained,constexpr, and use compiler overflow builtins where applicable.Reviewer guidance now codified
cpp/agents.mdadds guidance for integer arithmetic used in sizes, launches, and host indexing..coderabbit.yamlasks CodeRabbit to flag unchecked host-side arithmetic and silent narrowing in C++ paths.Why a runtime helper instead of compiler warnings
cuML already builds with
-Werror(-Wall -Werroron GCC,-Werror=all-warningson nvcc). However,-Walldoes not include the warnings that catch silent integer narrowing or implicit conversions:-Wall?-Wnarrowingint x{some_size_t})-Wconversionint x = some_size_t)-Wsign-conversionEnabling
-Wconversionis the eventual right answer, but it would require an exhaustive cleanup PR series. The checked helpers and CodeRabbit rule are the bridge until then.What is not changed
Closes #8256