Skip to content

Commit 76e02bc

Browse files
Amxxernestognw
Amxx
and
ernestognw
authored
Fix bug in Bytes.lastIndexOf when array is empty and position is not 2²⁵⁶-1 (#5797)
Co-authored-by: Ernesto García <[email protected]>
1 parent 101bbaf commit 76e02bc

File tree

4 files changed

+104
-14
lines changed

4 files changed

+104
-14
lines changed

‎.changeset/witty-hats-flow.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'openzeppelin-solidity': patch
3+
---
4+
5+
`Bytes`: Fix `lastIndexOf(bytes,byte,uint256)` with empty buffers and finite position to correctly return `type(uint256).max` instead of accessing uninitialized memory sections.

‎contracts/utils/Bytes.sol‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,7 @@ library Bytes {
5858
function lastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) internal pure returns (uint256) {
5959
unchecked {
6060
uint256 length = buffer.length;
61-
// NOTE here we cannot do `i = Math.min(pos + 1, length)` because `pos + 1` could overflow
62-
for (uint256 i = Math.min(pos, length - 1) + 1; i > 0; --i) {
61+
for (uint256 i = Math.min(Math.saturatingAdd(pos, 1), length); i > 0; --i) {
6362
if (bytes1(_unsafeReadBytesOffset(buffer, i - 1)) == s) {
6463
return i - 1;
6564
}

‎test/utils/Bytes.t.sol‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,76 @@ import {Bytes} from "@openzeppelin/contracts/utils/Bytes.sol";
99
contract BytesTest is Test {
1010
using Bytes for bytes;
1111

12+
// INDEX OF
13+
function testIndexOf(bytes memory buffer, bytes1 s) public pure {
14+
uint256 result = Bytes.indexOf(buffer, s);
15+
16+
if (buffer.length == 0) {
17+
// Case 0: buffer is empty
18+
assertEq(result, type(uint256).max);
19+
} else if (result == type(uint256).max) {
20+
// Case 1: search value could not be found
21+
for (uint256 i = 0; i < buffer.length; ++i) assertNotEq(buffer[i], s);
22+
} else {
23+
// Case 2: search value was found
24+
assertEq(buffer[result], s);
25+
// search value is not present anywhere before the found location
26+
for (uint256 i = 0; i < result; ++i) assertNotEq(buffer[i], s);
27+
}
28+
}
29+
30+
function testIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure {
31+
uint256 result = Bytes.indexOf(buffer, s, pos);
32+
33+
if (buffer.length == 0) {
34+
// Case 0: buffer is empty
35+
assertEq(result, type(uint256).max);
36+
} else if (result == type(uint256).max) {
37+
// Case 1: search value could not be found
38+
for (uint256 i = pos; i < buffer.length; ++i) assertNotEq(buffer[i], s);
39+
} else {
40+
// Case 2: search value was found
41+
assertEq(buffer[result], s);
42+
// search value is not present anywhere before the found location
43+
for (uint256 i = pos; i < result; ++i) assertNotEq(buffer[i], s);
44+
}
45+
}
46+
47+
function testLastIndexOf(bytes memory buffer, bytes1 s) public pure {
48+
uint256 result = Bytes.lastIndexOf(buffer, s);
49+
50+
if (buffer.length == 0) {
51+
// Case 0: buffer is empty
52+
assertEq(result, type(uint256).max);
53+
} else if (result == type(uint256).max) {
54+
// Case 1: search value could not be found
55+
for (uint256 i = 0; i < buffer.length; ++i) assertNotEq(buffer[i], s);
56+
} else {
57+
// Case 2: search value was found
58+
assertEq(buffer[result], s);
59+
// search value is not present anywhere after the found location
60+
for (uint256 i = result + 1; i < buffer.length; ++i) assertNotEq(buffer[i], s);
61+
}
62+
}
63+
64+
function testLastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure {
65+
uint256 result = Bytes.lastIndexOf(buffer, s, pos);
66+
67+
if (buffer.length == 0) {
68+
// Case 0: buffer is empty
69+
assertEq(result, type(uint256).max);
70+
} else if (result == type(uint256).max) {
71+
// Case 1: search value could not be found
72+
for (uint256 i = 0; i <= Math.min(pos, buffer.length - 1); ++i) assertNotEq(buffer[i], s);
73+
} else {
74+
// Case 2: search value was found
75+
assertEq(buffer[result], s);
76+
// search value is not present anywhere after the found location
77+
for (uint256 i = result + 1; i <= Math.min(pos, buffer.length - 1); ++i) assertNotEq(buffer[i], s);
78+
}
79+
}
80+
81+
// SLICES
1282
function testSliceWithStartOnly(bytes memory buffer, uint256 start) public pure {
1383
bytes memory originalBuffer = bytes.concat(buffer);
1484
bytes memory result = buffer.slice(start);

‎test/utils/Bytes.test.js‎

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,39 +28,55 @@ describe('Bytes', function () {
2828

2929
describe('indexOf', function () {
3030
it('first', async function () {
31-
expect(await this.mock.$indexOf(lorem, ethers.toBeHex(present))).to.equal(lorem.indexOf(present));
31+
await expect(this.mock.$indexOf(lorem, ethers.toBeHex(present))).to.eventually.equal(lorem.indexOf(present));
3232
});
3333

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

4244
it('absent', async function () {
43-
expect(await this.mock.$indexOf(lorem, ethers.toBeHex(absent))).to.equal(ethers.MaxUint256);
45+
await expect(this.mock.$indexOf(lorem, ethers.toBeHex(absent))).to.eventually.equal(ethers.MaxUint256);
46+
});
47+
48+
it('empty buffer', async function () {
49+
await expect(this.mock.$indexOf('0x', '0x00')).to.eventually.equal(ethers.MaxUint256);
50+
await expect(this.mock.$indexOf('0x', '0x00', ethers.Typed.uint256(17))).to.eventually.equal(ethers.MaxUint256);
4451
});
4552
});
4653

4754
describe('lastIndexOf', function () {
4855
it('first', async function () {
49-
expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(present))).to.equal(lorem.lastIndexOf(present));
56+
await expect(this.mock.$lastIndexOf(lorem, ethers.toBeHex(present))).to.eventually.equal(
57+
lorem.lastIndexOf(present),
58+
);
5059
});
5160

5261
it('from index', async function () {
5362
for (const start in Array(lorem.length + 10).fill()) {
5463
const index = lorem.lastIndexOf(present, start);
5564
const result = index === -1 ? ethers.MaxUint256 : index;
56-
expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start))).to.equal(
57-
result,
58-
);
65+
await expect(
66+
this.mock.$lastIndexOf(lorem, ethers.toBeHex(present), ethers.Typed.uint256(start)),
67+
).to.eventually.equal(result);
5968
}
6069
});
6170

6271
it('absent', async function () {
63-
expect(await this.mock.$lastIndexOf(lorem, ethers.toBeHex(absent))).to.equal(ethers.MaxUint256);
72+
await expect(this.mock.$lastIndexOf(lorem, ethers.toBeHex(absent))).to.eventually.equal(ethers.MaxUint256);
73+
});
74+
75+
it('empty buffer', async function () {
76+
await expect(this.mock.$lastIndexOf('0x', '0x00')).to.eventually.equal(ethers.MaxUint256);
77+
await expect(this.mock.$lastIndexOf('0x', '0x00', ethers.Typed.uint256(17))).to.eventually.equal(
78+
ethers.MaxUint256,
79+
);
6480
});
6581
});
6682

@@ -73,8 +89,8 @@ describe('Bytes', function () {
7389
})) {
7490
it(descr, async function () {
7591
const result = ethers.hexlify(lorem.slice(start));
76-
expect(await this.mock.$slice(lorem, start)).to.equal(result);
77-
expect(await this.mock.$splice(lorem, start)).to.equal(result);
92+
await expect(this.mock.$slice(lorem, start)).to.eventually.equal(result);
93+
await expect(this.mock.$splice(lorem, start)).to.eventually.equal(result);
7894
});
7995
}
8096
});
@@ -89,8 +105,8 @@ describe('Bytes', function () {
89105
})) {
90106
it(descr, async function () {
91107
const result = ethers.hexlify(lorem.slice(start, end));
92-
expect(await this.mock.$slice(lorem, start, ethers.Typed.uint256(end))).to.equal(result);
93-
expect(await this.mock.$splice(lorem, start, ethers.Typed.uint256(end))).to.equal(result);
108+
await expect(this.mock.$slice(lorem, start, ethers.Typed.uint256(end))).to.eventually.equal(result);
109+
await expect(this.mock.$splice(lorem, start, ethers.Typed.uint256(end))).to.eventually.equal(result);
94110
});
95111
}
96112
});

0 commit comments

Comments
 (0)