Skip to content

Fix invalid pointer arithmetic in Hash #1222

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
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

@davidben davidben commented Dec 24, 2024

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

davidben commented Dec 24, 2024

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 <[email protected]>
> Reviewed-by: Daniel Cheng <[email protected]>
> Auto-Submit: David Benjamin <[email protected]>
> Commit-Queue: David Benjamin <[email protected]>
> 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 <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nico Weber <[email protected]>
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 <[email protected]>
> Reviewed-by: Daniel Cheng <[email protected]>
> Auto-Submit: David Benjamin <[email protected]>
> Commit-Queue: David Benjamin <[email protected]>
> 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 <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nico Weber <[email protected]>
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 <[email protected]>
> Reviewed-by: Daniel Cheng <[email protected]>
> Auto-Submit: David Benjamin <[email protected]>
> Commit-Queue: David Benjamin <[email protected]>
> 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 <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Nico Weber <[email protected]>
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
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
@victorvianna victorvianna mentioned this pull request Jan 29, 2025
Merged
ybtsdst pushed a commit to ybtsdst/leveldb that referenced this pull request Apr 27, 2025
@davidben
Fix invalid pointer arithmetic in Hash (google#1222)
f540be1 
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)
ybtsdst pushed a commit to ybtsdst/leveldb that referenced this pull request Apr 27, 2025
@victorvianna
Reland changes accidentally reverted in 302786e
97eb508 
These changes
  1) 2cc36eb - "[jumbo] Add begin()/end() to Slice."
  2) 578eeb7 - "Fix invalid pointer arithmetic in Hash (google#1222)"
were committed in the public repository but never got imported to
the internal Google repository. Later, cl/713346733 landed in the
internal repo. When tooling published the internal change as
302786e ("Fix C++23 compilation errors in leveldb"), it
accidentally reverted commits (1) and (2).

This change re-commits a bundled version of (1) and (2) in the
public repo. This will then be imported to the private repo,
leaving the 2 in sync.
symious pushed a commit to VisitRe/leveldb that referenced this pull request Jul 28, 2025
@davidben @symious
Fix invalid pointer arithmetic in Hash (google#1222)
b2be119 
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)
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