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 out-of-bounds memory read in orc gpuEncodeOrcColumnData #9196

Merged

Conversation

davidwendt
Copy link
Contributor

Device memory read error found in gpuEncodeOrcColumnData when running ORC_TEST with compute-sanitizer.

[ RUN      ] OrcChunkedWriterTest.LargeTables
========= Invalid __global__ read of size 4 bytes
=========     at 0x8b0 in void cudf::io::orc::gpu::gpuEncodeOrcColumnData<int=512>(cudf::detail::base_2dspan<cudf::io::orc::gpu::EncChunk const ,cudf::device_span>,cudf::detail<cudf::io::orc::gpu::encoder_chunk_streams,cudf::io::orc::gpu::EncChunk const >)
=========     by thread (60,0,0) in block (255,0,0)
=========     Address 0x7fcd7a000000 is out of bounds
=========     Saved host backtrace up to driver entry point at kernel launch time
...

The was in the cudf::detail::get_mask_offset_word utility which may need to read multiple bitmask_type values (4-bytes == 32-bits) to satisfy the begin/end bit parameters. The source_end_bit is intended to be exclusive but the logic inadvertently reads the next bytemask_type from the input source null-mask on boundary cases like the one found in the gtest above. Here the source_begin_bit==480 and the source_end_bit==512 and because word_index(512) > word_index(480) the next read access is out of bounds. This PR fixed the logic in the utility by ensuring only the inclusive bits are verified to require and extra read from source.

The logic in cudf::io::orc::gpu::gpuEncodeOrcColumnData that calls this utility also required a fix where it always requested at least 32-bits regardless if it was out of bounds for source. This PR fixes the math logic to specify the correct end-bit value.

@davidwendt davidwendt added bug Something isn't working 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Sep 8, 2021
@davidwendt davidwendt self-assigned this Sep 8, 2021
@davidwendt davidwendt requested a review from a team as a code owner September 8, 2021 16:09
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!
My struct PR includes some refactoring of this code, will make sure to merge with this PR correctly.

cpp/src/io/orc/stripe_enc.cu Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@b10835c). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head d13101f differs from pull request most recent head d4c80c0. Consider uploading reports for the commit d4c80c0 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #9196   +/-   ##
===============================================
  Coverage                ?   10.77%           
===============================================
  Files                   ?      115           
  Lines                   ?    19138           
  Branches                ?        0           
===============================================
  Hits                    ?     2062           
  Misses                  ?    17076           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b10835c...d4c80c0. Read the comment docs.

Copy link
Contributor

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

I wonder if there is a unit test to catch the already fixed bug.

@vuule
Copy link
Contributor

vuule commented Sep 15, 2021

I wonder if there is a unit test to catch the already fixed bug.

I don't think so, since the part that was read out-of-bounds is never used.

@vuule
Copy link
Contributor

vuule commented Sep 15, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 015f15c into rapidsai:branch-21.10 Sep 15, 2021
@davidwendt davidwendt deleted the bug-memcheck-read-orc-stripe-enc branch September 16, 2021 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants