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.

Sign up for GitHub

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

Jump to bottom

Fix invalid pointer arithmetic in Hash #1222

Merged
merged 1 commit into from
Jan 2, 2025
Merged

Fix invalid pointer arithmetic in Hash #1222

merged 1 commit into from
Jan 2, 2025

Conversation

davidben
Copy link
Contributor

It is UB to exceed the bounds of the buffer when doing pointer arithemetic. That means the following is not a valid bounds check:

if (start + 4 <= limit)

Because if we were at the end of the buffer, we wouldn't be allowed to add 4 anyway. Instead, this must be written as:

if (limit - start >= 4)

Basic forms of this issue are flagged by UBSan. If building with -fsanitize=undefined, the following test trips an error:

[ RUN      ] HASH.SignedUnsignedIssue
.../leveldb/util/hash.cc:30:15: runtime error: applying non-zero offset 4 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/davidben/leveldb/util/hash.cc:30:15 in
[       OK ] HASH.SignedUnsignedIssue (1 ms)

@davidben
Fix invalid pointer arithmetic in Hash
84e635f 
It is UB to exceed the bounds of the buffer when doing pointer
arithemetic. That means the following is not a valid bounds check:

    if (start + 4 <= limit)

Because if we were at the end of the buffer, we wouldn't be
allowed to add 4 anyway. Instead, this must be written as:

    if (limit - start >= 4)

Basic forms of this issue are flagged by UBSan. If building with
-fsanitize=undefined, the following test trips an error:

    [ RUN      ] HASH.SignedUnsignedIssue
    .../leveldb/util/hash.cc:30:15: runtime error: applying non-zero offset 4 to null pointer
    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/davidben/leveldb/util/hash.cc:30:15 in
    [       OK ] HASH.SignedUnsignedIssue (1 ms)
@davidben
Copy link
Contributor Author

CC @a-sully just because I noticed this repo isn't getting that much activity and you last merged something here. 😄

aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 24, 2024
@davidben
Reland "Add -fsanitize=pointer-overflow to the UBSan config"
fd7de0b 
This is a reland of commit 103f0ee

The fix for leveldb is google/leveldb#1222,
but add a suppression for now.

Original change's description:
> Add -fsanitize=pointer-overflow to the UBSan config
>
> This required adding a couple suppressions for issues. Also fixing
> undefined behavior in BufferIteratorTest.ObjectSizeOverflow, which seems
> to have not been testing SIZE_MAX at all, and just when the buffer
> didn't have enough room.
>
> Bug: 40942951, 384391188, 385062729, 385155394
> Change-Id: If6defef3fbc4977632fccc63049901836d4a5347
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_ubsan_rel_ng
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6108769
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Auto-Submit: David Benjamin <davidben@chromium.org>
> Commit-Queue: David Benjamin <davidben@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1399851}

Bug: 40942951, 384391188, 385062729, 385155394
Change-Id: I535d865661be49f11a291c62671557f7541b2bb9
Cq-Include-Trybots: luci.chromium.try:linux_chromium_ubsan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6120908
Auto-Submit: David Benjamin <davidben@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1399949}
github-actions bot pushed a commit to kaidokert/chrome_base_mirror that referenced this pull request Dec 24, 2024
@davidben @copybara-github
Reland "Add -fsanitize=pointer-overflow to the UBSan config"
48c2dc1 
This is a reland of commit 103f0ee63afb8a42af04c9a152dc938e3571c128

The fix for leveldb is google/leveldb#1222,
but add a suppression for now.

Original change's description:
> Add -fsanitize=pointer-overflow to the UBSan config
>
> This required adding a couple suppressions for issues. Also fixing
> undefined behavior in BufferIteratorTest.ObjectSizeOverflow, which seems
> to have not been testing SIZE_MAX at all, and just when the buffer
> didn't have enough room.
>
> Bug: 40942951, 384391188, 385062729, 385155394
> Change-Id: If6defef3fbc4977632fccc63049901836d4a5347
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_ubsan_rel_ng
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6108769
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Auto-Submit: David Benjamin <davidben@chromium.org>
> Commit-Queue: David Benjamin <davidben@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1399851}

Bug: 40942951, 384391188, 385062729, 385155394
Change-Id: I535d865661be49f11a291c62671557f7541b2bb9
Cq-Include-Trybots: luci.chromium.try:linux_chromium_ubsan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6120908
Auto-Submit: David Benjamin <davidben@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1399949}
NOKEYCHECK=True
GitOrigin-RevId: fd7de0b84913bda60edcf21ab6c7c0cd017b1d70
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.external.libchrome that referenced this pull request Jan 1, 2025
@davidben
Reland "Add -fsanitize=pointer-overflow to the UBSan config"
73c2107 
This is a reland of commit 103f0ee63afb8a42af04c9a152dc938e3571c128

The fix for leveldb is google/leveldb#1222,
but add a suppression for now.

Original change's description:
> Add -fsanitize=pointer-overflow to the UBSan config
>
> This required adding a couple suppressions for issues. Also fixing
> undefined behavior in BufferIteratorTest.ObjectSizeOverflow, which seems
> to have not been testing SIZE_MAX at all, and just when the buffer
> didn't have enough room.
>
> Bug: 40942951, 384391188, 385062729, 385155394
> Change-Id: If6defef3fbc4977632fccc63049901836d4a5347
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_ubsan_rel_ng
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6108769
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Auto-Submit: David Benjamin <davidben@chromium.org>
> Commit-Queue: David Benjamin <davidben@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1399851}

Bug: 40942951, 384391188, 385062729, 385155394
Change-Id: I535d865661be49f11a291c62671557f7541b2bb9
Cq-Include-Trybots: luci.chromium.try:linux_chromium_ubsan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6120908
Auto-Submit: David Benjamin <davidben@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1399949}


CrOS-Libchrome-Original-Commit: fd7de0b84913bda60edcf21ab6c7c0cd017b1d70
@a-sully a-sully merged commit 578eeb7 into google:main Jan 2, 2025
9 checks passed
ogsts pushed a commit to ubiquiti/ubnt_libjingle_component_src_build that referenced this pull request Jan 12, 2025
@davidben @copybara-github
Reland "Add -fsanitize=pointer-overflow to the UBSan config"
8f6742f 
This is a reland of commit 103f0ee63afb8a42af04c9a152dc938e3571c128

The fix for leveldb is google/leveldb#1222,
but add a suppression for now.

Original change's description:
> Add -fsanitize=pointer-overflow to the UBSan config
>
> This required adding a couple suppressions for issues. Also fixing
> undefined behavior in BufferIteratorTest.ObjectSizeOverflow, which seems
> to have not been testing SIZE_MAX at all, and just when the buffer
> didn't have enough room.
>
> Bug: 40942951, 384391188, 385062729, 385155394
> Change-Id: If6defef3fbc4977632fccc63049901836d4a5347
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_ubsan_rel_ng
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6108769
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Auto-Submit: David Benjamin <davidben@chromium.org>
> Commit-Queue: David Benjamin <davidben@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1399851}

Bug: 40942951, 384391188, 385062729, 385155394
Change-Id: I535d865661be49f11a291c62671557f7541b2bb9
Cq-Include-Trybots: luci.chromium.try:linux_chromium_ubsan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6120908
Auto-Submit: David Benjamin <davidben@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1399949}
NOKEYCHECK=True
GitOrigin-RevId: fd7de0b84913bda60edcf21ab6c7c0cd017b1d70
maflcko pushed a commit to maflcko/leveldb-subtree that referenced this pull request Jan 14, 2025
@davidben
Fix invalid pointer arithmetic in Hash (google#1222)
a8844b2 
It is UB to exceed the bounds of the buffer when doing pointer
arithemetic. That means the following is not a valid bounds check:

    if (start + 4 <= limit)

Because if we were at the end of the buffer, we wouldn't be
allowed to add 4 anyway. Instead, this must be written as:

    if (limit - start >= 4)

Basic forms of this issue are flagged by UBSan. If building with
-fsanitize=undefined, the following test trips an error:

    [ RUN      ] HASH.SignedUnsignedIssue
    .../leveldb/util/hash.cc:30:15: runtime error: applying non-zero offset 4 to null pointer
    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/davidben/leveldb/util/hash.cc:30:15 in
    [       OK ] HASH.SignedUnsignedIssue (1 ms)

(cherry picked from commit 578eeb7)
fanquake added a commit to bitcoin-core/leveldb-subtree that referenced this pull request Jan 16, 2025
@fanquake
Merge #46: Fix invalid pointer arithmetic in Hash (google#1222)
04b5790 
a8844b2 Fix invalid pointer arithmetic in Hash (google#1222) (David Benjamin)

Pull request description:

  It is UB to exceed the bounds of the buffer when doing pointer arithemetic. That means the following is not a valid bounds check:

      if (start + 4 <= limit)

  Because if we were at the end of the buffer, we wouldn't be allowed to add 4 anyway. Instead, this must be written as:

      if (limit - start >= 4)

  Basic forms of this issue are flagged by UBSan. If building with -fsanitize=undefined, the following test trips an error:

      [ RUN      ] HASH.SignedUnsignedIssue
      .../leveldb/util/hash.cc:30:15: runtime error: applying non-zero offset 4 to null pointer
      SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/davidben/leveldb/util/hash.cc:30:15 in
      [       OK ] HASH.SignedUnsignedIssue (1 ms)

  (cherry picked from commit 578eeb7)

ACKs for top commit:
  l0rinc:
    ACK a8844b2
  fanquake:
    ACK a8844b2

Tree-SHA512: 4e3e7aa680ec0a7ed759dd47318876e78cc04bd65456b7b0e41f7c29f1f70cf0f9568b7f32056bedae8492ee7fe5510f25d74a48a27518185248c5c34e54fa2f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davidben @a-sully