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

Fix integer overflow in partition scatter_map construction #13272

Merged
merged 6 commits into from
May 5, 2023

Conversation

wence-
Copy link
Contributor

@wence- wence- commented May 2, 2023

Description

Although the input partition map may be any valid integral type, the
intermediate scatter map should have the same type as a valid row
index (and the offsets histogram), namely size_type. Previously, the
scatter map was created with the same integer type as the partition
map, which can result in integer overflow, and incorrect results, when
the partition map is a narrow integral type and the input table has
more rows than the width of the type.

On the Python side, this adds validation of the user
partition_map input to ensure that all entries are in range,
avoiding potential out of bounds memory accesses.

Closes #7513.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

wence- added 2 commits May 2, 2023 15:56
Additionally, ensure that the partition_map has valid entries,
otherwise one can end up with out of bounds memory reads in libcudf.

Closes the part of rapidsai#7513 related to invalid inputs.
Although the input partition map may be any valid integral type, the
intermediate scatter map should have the same type as a valid row
index (and the offsets histogram), namely `size_type`. Previously, the
scatter map was created with the same integer type as the partition
map, which can result in integer overflow, and incorrect results, when
the partition map is a narrow integral type and the input table has
more rows than the width of the type.

Closes rapidsai#7513.
@wence- wence- requested review from a team as code owners May 2, 2023 15:02
@github-actions github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels May 2, 2023
@wence- wence- added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. labels May 2, 2023
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Approving for C++

@github-actions github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels May 3, 2023
@wence-
Copy link
Contributor Author

wence- commented May 3, 2023

Merged trunk so hopefully the unrelated python test fails will be gone.

@ttnghia
Copy link
Contributor

ttnghia commented May 3, 2023

I think the failed python tests are relevant:

/opt/conda/envs/test/lib/python3.9/site-packages/cudf/core/dataframe.py:2290: in scatter_by_map
    partitioned_columns, output_offsets = libcudf.partitioning.partition(
/opt/conda/envs/test/lib/python3.9/contextlib.py:79: in inner
    return func(*args, **kwds)
partitioning.pyx:59: in cudf._lib.partitioning.partition
    ???
/opt/conda/envs/test/lib/python3.9/site-packages/cudf/core/scalar.py:279: in __bool__
    return bool(self.value)
....

@wence-
Copy link
Contributor Author

wence- commented May 3, 2023

I think the failed python tests are relevant:

/opt/conda/envs/test/lib/python3.9/site-packages/cudf/core/dataframe.py:2290: in scatter_by_map
    partitioned_columns, output_offsets = libcudf.partitioning.partition(
/opt/conda/envs/test/lib/python3.9/contextlib.py:79: in inner
    return func(*args, **kwds)
partitioning.pyx:59: in cudf._lib.partitioning.partition
    ???
/opt/conda/envs/test/lib/python3.9/site-packages/cudf/core/scalar.py:279: in __bool__
    return bool(self.value)
....

Ah, I didn't think about nulls in my error checking. Will look tomorrow.

@wence-
Copy link
Contributor Author

wence- commented May 4, 2023

I think the failed python tests are relevant:

/opt/conda/envs/test/lib/python3.9/site-packages/cudf/core/dataframe.py:2290: in scatter_by_map
    partitioned_columns, output_offsets = libcudf.partitioning.partition(
/opt/conda/envs/test/lib/python3.9/contextlib.py:79: in inner
    return func(*args, **kwds)
partitioning.pyx:59: in cudf._lib.partitioning.partition
    ???
/opt/conda/envs/test/lib/python3.9/site-packages/cudf/core/scalar.py:279: in __bool__
    return bool(self.value)
....

Ah, I didn't think about nulls in my error checking. Will look tomorrow.

OK, should be fixed...

@wence-
Copy link
Contributor Author

wence- commented May 5, 2023

/merge

@rapids-bot rapids-bot bot merged commit a208d21 into rapidsai:branch-23.06 May 5, 2023
@wence- wence- deleted the wence/fix/issue-7513 branch May 5, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] scatter_by_map does not work correctly
5 participants