Repository navigation
Conversation
Functions taking an axis used it to index shape, strides or slice containers without checking it. xt::argmax(a, 2) on a 2-D xtensor wrote past the std::array holding the result shape (stack-buffer-overflow under AddressSanitizer). diff, trapz, unwrap, cumsum, average, sort, partition, median, quantile, fft, expand_dims, flip, repeat, squeeze, concatenate, stack, the axis iterators and the blockwise reducers read or wrote out of bounds the same way. normalize_axis did no validation, and its container overloads only had an XTENSOR_ASSERT, which is off in release builds. It also took the axis as std::ptrdiff_t, so an unsigned std::size_t(-1) became the last axis instead of an error. normalize_axis now throws std::runtime_error when the axis is not in [-dim, dim). That is the exception type reducers, roll, split and squeeze already throw for a bad axis. It is a template over the axis type, and negative values count from the end only for signed types. The container overloads and forward_normalize run the same check. expand_dims and stack insert a new axis, so they validate against dim + 1. Functions that indexed with the raw axis now go through normalize_axis: expand_dims, split, flip, repeat, squeeze, concatenate, stack, axis_begin, axis_end, axis_slice_begin, axis_slice_end and the blockwise reducers. sort, partition and argpartition check the axis before their 1-D shortcut, which used to ignore it. roll and split drop their own bounds check in favor of the shared one. argmin and argmax validate the axis inside arg_func_impl, next to the code that builds the result shape. That code now uses a loop that skips the axis instead of two std::copy calls, which removes the GCC -Warray-bounds warnings on alt_shape. squeeze and the blockwise reducers also reject repeated axes, because they size the result shape from the number of axes. squeeze accepts negative axes. average throws when 1-D weights come with anything other than exactly one axis, instead of reading ax[0].
Contributor
Merging this PR will regress 1 benchmark2 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
normalize_axisdoc comment is updated).Description
normalize_axisdid not check its input, and several functions taking an axis did not call it. An out-of-range axis was then used to index shape, strides or slice containers:AddressSanitizer reports buffer overflows of this kind in
argmax,diff,expand_dims,stack,axis_slice_beginandfft. The container overloads ofnormalize_axisonly had anXTENSOR_ASSERT, which is off in release builds.normalize_axisnow throwsstd::runtime_errorwhen the axis is outside[-dim, dim), the type reducers,roll,splitandsqueezealready throw. It is a template on the axis type, sostd::size_t(-1)is rejected instead of becoming the last axis.expand_dimsandstackcheck againstdim + 1since they insert an axis. Functions that skipped the check (flip,repeat,squeeze,concatenate,stack,expand_dims, the axis iterators, the blockwise reducers, and the 1-D shortcut ofsort/argsort/partition/argpartition) now call it. Inargmin/argmax, the result shape is built with a loop, which also removes the GCC 13/15-Warray-boundswarnings inxsort.hpp.Behavior changes:
squeezeand the blockwise reducers accept negative axes and reject repeated ones;averagewith 1-D weights requires exactly one axis;rollandsplitchanges, not the exception type.The tests added in
test_xutils,test_xsort,test_xmath,test_xmanipulation,test_xbuilder,test_xaxis_iterator,test_xaxis_slice_iterator,test_xfftandtest_xblockwise_reducerfail on master and pass with the fix, also under AddressSanitizer and UBSan.