Skip to content

crush: avoid out-of-bound access and simplify enlarging buckets #57786

Merged
Svelar merged 1 commit intoceph:mainfrom
Svelar:asan_cli_test
Aug 5, 2024
Merged

crush: avoid out-of-bound access and simplify enlarging buckets #57786
Svelar merged 1 commit intoceph:mainfrom
Svelar:asan_cli_test

Conversation

@Svelar
Copy link
Member

@Svelar Svelar commented May 30, 2024 •
edited
Loading

When sanitizer is enabled, a part of 'run-cli-tests' output shows,

=================================================================
  ==1263095==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c00000c000 at pc 0x7f80a4b0a040 bp 0x7ffe3176d550 sp 0x7ffe3176d548
  READ of size 8 at 0x60c00000c000 thread T0
      #0 0x7f80a4b0a03f in CrushWrapper::get_new_bucket_id() /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:2189:10
      #1 0x7f80a4b03f20 in CrushWrapper::reclassify(ceph::common::CephContext*, std::ostream&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > const&) /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:1957:20
      #2 0x55d567dfbcec in main /home/jenkins-build/build/workspace/ceph-pull-requests/src/tools/crushtool.cc:1215:19
      #3 0x7f80a06c7d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
      #4 0x7f80a06c7e3f in __libc_start_main csu/../csu/libc-start.c:392:3
      #5 0x55d567d2b4d4 in _start (/home/jenkins-build/build/workspace/ceph-pull-requests/build/bin/crushtool+0xb54d4) (BuildId: ce3df2d268a883ca3965158085f32e534cbedaf5)

  0x60c00000c000 is located 0 bytes to the right of 128-byte region [0x60c00000bf80,0x60c00000c000)
  allocated by thread T0 here:
      #0 0x55d567dae508 in __interceptor_calloc (/home/jenkins-build/build/workspace/ceph-pull-requests/build/bin/crushtool+0x138508) (BuildId: ce3df2d268a883ca3965158085f32e534cbedaf5)
      #1 0x7f80a4b164cf in CrushWrapper::decode(ceph::buffer::v15_2_0::list::iterator_impl<true>&) /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:3267:38
      #2 0x55d567df69eb in main /home/jenkins-build/build/workspace/ceph-pull-requests/src/tools/crushtool.cc:919:13
      #3 0x7f80a06c7d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

  SUMMARY: AddressSanitizer: heap-buffer-overflow /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:2189:10 in CrushWrapper::get_new_bucket_id()

fixes: https://tracker.ceph.com/issues/66861

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@Svelar Svelar requested a review from a team as a code owner May 30, 2024 08:24
@github-actions github-actions bot added the core label May 30, 2024
@Svelar Svelar mentioned this pull request May 30, 2024
Draft
14 tasks
@Svelar
Copy link
Member Author

Svelar commented May 31, 2024

jenkins test windows

@Svelar
Copy link
Member Author

Svelar commented May 31, 2024

jenkins test make check

tchaikov
tchaikov approved these changes May 31, 2024
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

lgtm

tchaikov
tchaikov reviewed May 31, 2024
View reviewed changes
tchaikov
tchaikov requested changes May 31, 2024
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

"test/ run-cli-tests:" this prefix is misleading. should use "crush". and i think this change should be backported. as the out-of-bound access is UB.

tchaikov
tchaikov reviewed Jun 25, 2024
View reviewed changes
@tchaikov
Copy link
Contributor

tchaikov commented Jun 25, 2024

@Svelar ping?

@Svelar Svelar force-pushed the asan_cli_test branch 2 times, most recently from f617ae5 to 9e886bf Compare July 4, 2024 08:21
@Svelar Svelar requested a review from tchaikov July 5, 2024 06:41
tchaikov
tchaikov reviewed Jul 8, 2024
View reviewed changes
tchaikov
tchaikov reviewed Jul 8, 2024
View reviewed changes
@tchaikov
Copy link
Contributor

tchaikov commented Jul 8, 2024

"test/ run-cli-tests:" this prefix is misleading. should use "crush". and i think this change should be backported. as the out-of-bound access is UB.

could you please reconsider this comment?

@Svelar
crush: avoid out-of-bound access and simplify enlarging buckets
e6468f3 
When sanitizer is enabled, a part of 'run-cli-tests' output shows,

```
=================================================================
  ==1263095==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c00000c000 at pc 0x7f80a4b0a040 bp 0x7ffe3176d550 sp 0x7ffe3176d548
  READ of size 8 at 0x60c00000c000 thread T0
      #0 0x7f80a4b0a03f in CrushWrapper::get_new_bucket_id() /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:2189:10
      #1 0x7f80a4b03f20 in CrushWrapper::reclassify(ceph::common::CephContext*, std::ostream&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > const&) /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:1957:20
      #2 0x55d567dfbcec in main /home/jenkins-build/build/workspace/ceph-pull-requests/src/tools/crushtool.cc:1215:19
      #3 0x7f80a06c7d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
      #4 0x7f80a06c7e3f in __libc_start_main csu/../csu/libc-start.c:392:3
      #5 0x55d567d2b4d4 in _start (/home/jenkins-build/build/workspace/ceph-pull-requests/build/bin/crushtool+0xb54d4) (BuildId: ce3df2d268a883ca3965158085f32e534cbedaf5)

  0x60c00000c000 is located 0 bytes to the right of 128-byte region [0x60c00000bf80,0x60c00000c000)
  allocated by thread T0 here:
      #0 0x55d567dae508 in __interceptor_calloc (/home/jenkins-build/build/workspace/ceph-pull-requests/build/bin/crushtool+0x138508) (BuildId: ce3df2d268a883ca3965158085f32e534cbedaf5)
      #1 0x7f80a4b164cf in CrushWrapper::decode(ceph::buffer::v15_2_0::list::iterator_impl<true>&) /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:3267:38
      #2 0x55d567df69eb in main /home/jenkins-build/build/workspace/ceph-pull-requests/src/tools/crushtool.cc:919:13
      #3 0x7f80a06c7d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

  SUMMARY: AddressSanitizer: heap-buffer-overflow /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:2189:10 in CrushWrapper::get_new_bucket_id()
```

fixes: https://tracker.ceph.com/issues/66861

Signed-off-by: Rongqi Sun <sunrongqi@huawei.com>
@Svelar Svelar changed the title test/ run-cli-tests: check id first crush: avoid out-of-bound access and simplify enlarging buckets Jul 8, 2024
@Svelar
Copy link
Member Author

Svelar commented Jul 8, 2024

"test/ run-cli-tests:" this prefix is misleading. should use "crush". and i think this change should be backported. as the out-of-bound access is UB.

could you please reconsider this comment?

Sorry for missing it, adjusted~

tchaikov
tchaikov approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

lgtm

@Svelar
Copy link
Member Author

Svelar commented Jul 9, 2024

jenkins test make check arm64

pereman2
pereman2 reviewed Jul 29, 2024
View reviewed changes
Copy link
Contributor

@pereman2 pereman2 left a comment

Choose a reason for hiding this comment

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

https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues67028 rados approved

@athanatos athanatos self-requested a review August 3, 2024 00:04
athanatos
athanatos approved these changes Aug 3, 2024
View reviewed changes
Copy link
Contributor

@athanatos athanatos left a comment

Choose a reason for hiding this comment

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

Looks right to me.

@Svelar Svelar merged commit 9c7e72c into ceph:main Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tchaikov tchaikov tchaikov approved these changes

@athanatos athanatos athanatos approved these changes

@pereman2 pereman2 pereman2 approved these changes

Assignees

No one assigned

Labels

cleanup core needs-qa tests wip-yuri4-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Svelar @tchaikov @athanatos @pereman2 @ljflores