Skip to content

Fix bug in Bytes.lastIndexOf when array is empty and position is not 2²⁵⁶-1 #5797

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

Merged
merged 8 commits into from
Jul 16, 2025
Merged

Fix bug in Bytes.lastIndexOf when array is empty and position is not 2²⁵⁶-1 #5797

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c62f26d
Fix bug in Bytes.lastIndexOf when array is empty and position is not …
Amxx Jul 15, 2025
aa1cdb8
add changeset
Amxx Jul 15, 2025
29b1b35
branchless
Amxx Jul 15, 2025
edf974e
simplify fuzz cases
Amxx Jul 15, 2025
655c3c0
independant fuzz tests of and without pos
Amxx Jul 15, 2025
cfbb2e6
Update .changeset/witty-hats-flow.md
ernestognw Jul 15, 2025
df473af
use Math.saturatingAdd to more closelly match original syntax
Amxx Jul 16, 2025
6eb86f4
minimize changes
Amxx Jul 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/witty-hats-flow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': patch
---

`Bytes`: Fix an issue when calling `lastIndexOf(bytes,byte,uint256)` with an empty buffer and a lookup position that is not 2²⁵⁶-1.
4 changes: 2 additions & 2 deletions contracts/utils/Bytes.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ library Bytes {
function lastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) internal pure returns (uint256) {
unchecked {
uint256 length = buffer.length;
// NOTE here we cannot do `i = Math.min(pos + 1, length)` because `pos + 1` could overflow
for (uint256 i = Math.min(pos, length - 1) + 1; i > 0; --i) {
uint256 end = Math.ternary(pos == type(uint256).max, length, Math.min(pos + 1, length));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might be more readable, and just as effective.

Suggested change
uint256 end = Math.ternary(pos == type(uint256).max, length, Math.min(pos + 1, length));
uint256 end = Math.min(Math.saturatingAdd(pos, 1), length);

@ernestognw @vesselinux @levi-sledd WDYT ?

Copy link

Choose a reason for hiding this comment

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

This is indeed equivalent and more readable.

Copy link

Choose a reason for hiding this comment

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

@Amxx Ahh, sorry to say but I find this to be even less readable haha. That's maybe because the overflow logic is buried inside the saturatingAdd thus making handling of edge cases less obvious. At the same time, I fully accept the arguments against my if-else suggestion.

The above said, I appreciate that readability is subjective, so feel free to adopt whichever of the two suggested variants the Contracts team like best. Indeed both are functionally equivalent and that's what matters at the end of the day.

Copy link
Member

Choose a reason for hiding this comment

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

the overflow logic is buried inside the saturatingAdd thus making handling of edge cases less obvious

Interesting point of view. imo "burying the logic" should be the goal of the saturatingAdd function (and generally for any other library fn). In fact, I do prefer the saturatingAdd given I'd recommend using it to other developers in a similar situation as this.

for (uint256 i = end; i > 0; --i) {
if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) {
return i - 1;
}
Expand Down
70 changes: 70 additions & 0 deletions test/utils/Bytes.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,76 @@ import {Bytes} from "@openzeppelin/contracts/utils/Bytes.sol";
contract BytesTest is Test {
using Bytes for bytes;

// INDEX OF
function testIndexOf(bytes memory buffer, bytes1 s) public pure {
uint256 result = Bytes.indexOf(buffer, s);

if (buffer.length == 0) {
// Case 0: buffer is empty
assertEq(result, type(uint256).max);
} else if (result == type(uint256).max) {
// Case 1: search value could not be found
for (uint256 i = 0; i < buffer.length; ++i) assertNotEq(buffer[i], s);
} else {
// Case 2: search value was found
assertEq(buffer[result], s);
// search value is not present anywhere before the found location
for (uint256 i = 0; i < result; ++i) assertNotEq(buffer[i], s);
}
}

function testIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure {
uint256 result = Bytes.indexOf(buffer, s, pos);

if (buffer.length == 0) {
// Case 0: buffer is empty
assertEq(result, type(uint256).max);
} else if (result == type(uint256).max) {
// Case 1: search value could not be found
for (uint256 i = pos; i < buffer.length; ++i) assertNotEq(buffer[i], s);
} else {
// Case 2: search value was found
assertEq(buffer[result], s);
// search value is not present anywhere before the found location
for (uint256 i = pos; i < result; ++i) assertNotEq(buffer[i], s);
}
}

function testLastIndexOf(bytes memory buffer, bytes1 s) public pure {
uint256 result = Bytes.lastIndexOf(buffer, s);

if (buffer.length == 0) {
// Case 0: buffer is empty
assertEq(result, type(uint256).max);
} else if (result == type(uint256).max) {
// Case 1: search value could not be found
for (uint256 i = 0; i < buffer.length; ++i) assertNotEq(buffer[i], s);
} else {
// Case 2: search value was found
assertEq(buffer[result], s);
// search value is not present anywhere after the found location
for (uint256 i = result + 1; i < buffer.length; ++i) assertNotEq(buffer[i], s);
}
}

function testLastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure {
uint256 result = Bytes.lastIndexOf(buffer, s, pos);

if (buffer.length == 0) {
// Case 0: buffer is empty
assertEq(result, type(uint256).max);
} else if (result == type(uint256).max) {
// Case 1: search value could not be found
for (uint256 i = 0; i <= Math.min(pos, buffer.length - 1); ++i) assertNotEq(buffer[i], s);
} else {
// Case 2: search value was found
assertEq(buffer[result], s);
// search value is not present anywhere after the found location
for (uint256 i = result + 1; i <= Math.min(pos, buffer.length - 1); ++i) assertNotEq(buffer[i], s);
}
}

// SLICES
function testSliceWithStartOnly(bytes memory buffer, uint256 start) public pure {
bytes memory originalBuffer = bytes.concat(buffer);
bytes memory result = buffer.slice(start);
Expand Down
40 changes: 28 additions & 12 deletions test/utils/Bytes.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,39 +28,55 @@ describe('Bytes', function () {

describe('indexOf', function () {
it('first', async function () {
expect(await this.mock.$indexOf(lorem, ethers.toBeHex(present))).to.equal(lorem.indexOf(present));
await expect(this.mock.$indexOf(lorem, ethers.toBeHex(present))).to.eventually.equal(lorem.indexOf(present));
});

it('from index', async function () {
for (const start in Array(lorem.length + 10).fill()) {
const index = lorem.indexOf(present, start);
const result = index === -1 ? ethers.MaxUint256 : index;
expect(await this.mock.$indexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start))).to.equal(result);
await expect(
this.mock.$indexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start)),
).to.eventually.equal(result);
}
});

it('absent', async function () {
expect(await this.mock.$indexOf(lorem, ethers.toBeHex(absent))).to.equal(ethers.MaxUint256);
await expect(this.mock.$indexOf(lorem, ethers.toBeHex(absent))).to.eventually.equal(ethers.MaxUint256);
});

it('empty buffer', async function () {
await expect(this.mock.$indexOf('0x', '0x00')).to.eventually.equal(ethers.MaxUint256);
await expect(this.mock.$indexOf('0x', '0x00', ethers.Typed.uint256(17))).to.eventually.equal(ethers.MaxUint256);
});
});

describe('lastIndexOf', function () {
it('first', async function () {
expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(present))).to.equal(lorem.lastIndexOf(present));
await expect(this.mock.$lastIndexOf(lorem, ethers.toBeHex(present))).to.eventually.equal(
lorem.lastIndexOf(present),
);
});

it('from index', async function () {
for (const start in Array(lorem.length + 10).fill()) {
const index = lorem.lastIndexOf(present, start);
const result = index === -1 ? ethers.MaxUint256 : index;
expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start))).to.equal(
result,
);
await expect(
this.mock.$lastIndexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start)),
).to.eventually.equal(result);
}
});

it('absent', async function () {
expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(absent))).to.equal(ethers.MaxUint256);
await expect(this.mock.$lastIndexOf(lorem, ethers.toBeHex(absent))).to.eventually.equal(ethers.MaxUint256);
});

it('empty buffer', async function () {
await expect(this.mock.$lastIndexOf('0x', '0x00')).to.eventually.equal(ethers.MaxUint256);
await expect(this.mock.$lastIndexOf('0x', '0x00', ethers.Typed.uint256(17))).to.eventually.equal(
ethers.MaxUint256,
);
});
});

Expand All @@ -73,8 +89,8 @@ describe('Bytes', function () {
})) {
it(descr, async function () {
const result = ethers.hexlify(lorem.slice(start));
expect(await this.mock.$slice(lorem, start)).to.equal(result);
expect(await this.mock.$splice(lorem, start)).to.equal(result);
await expect(this.mock.$slice(lorem, start)).to.eventually.equal(result);
await expect(this.mock.$splice(lorem, start)).to.eventually.equal(result);
});
}
});
Expand All @@ -89,8 +105,8 @@ describe('Bytes', function () {
})) {
it(descr, async function () {
const result = ethers.hexlify(lorem.slice(start, end));
expect(await this.mock.$slice(lorem, start, ethers.Typed.uint256(end))).to.equal(result);
expect(await this.mock.$splice(lorem, start, ethers.Typed.uint256(end))).to.equal(result);
await expect(this.mock.$slice(lorem, start, ethers.Typed.uint256(end))).to.eventually.equal(result);
await expect(this.mock.$splice(lorem, start, ethers.Typed.uint256(end))).to.eventually.equal(result);
});
}
});
Expand Down