[sandbox] Use SafeHeapObjectSize in FixedIntegerArray and PodArray [v8/v8 : main]
Arash Kazemi (Gerrit)
Arash Kazemi added 1 comment![]( "Open in Gerrit")
Related details
- Maksim Ivanov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 5 comments![]( "Open in Gerrit")
thanks!
for (uint32_t i = 0; i < static_cast<uint32_t>(n_blocks()); ++i) {Do we want to cache this in a variable to avoid repetitive casts everywhere? Or a follow-up CL will come soon that will update `n_blocks()` to return `uint32_t`, making these casts unnecessary here?
CHECK(!base::bits::SignedMulOverflow32(length, sizeof(T), &byte_length));Do we keep the signed arithmetics here because we want the byte size to not exceed `kMaxInt`? In that case, please add a comment to document that intention.
DCHECK_LE(index, this->length().value());`DCHECK_LT`? Ditto for `set()`.
uint32_t length,Would it make sense to change these construction size args to `SafeHeapObjectSize` as well, so that the compiler flags attempts to downcast from 64-bit sizes here?
Related details
- Arash Kazemi
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi added 4 comments![]( "Open in Gerrit")
for (uint32_t i = 0; i < static_cast<uint32_t>(n_blocks()); ++i) {Do we want to cache this in a variable to avoid repetitive casts everywhere? Or a follow-up CL will come soon that will update `n_blocks()` to return `uint32_t`, making these casts unnecessary here?
Yes, I have cached the `n_blocks()` value into a variable since we read it multiple times.
CHECK(!base::bits::SignedMulOverflow32(length, sizeof(T), &byte_length));Do we keep the signed arithmetics here because we want the byte size to not exceed `kMaxInt`? In that case, please add a comment to document that intention.
The size of a FixedArray should be below kMaxInt: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/fixed-array.h;l=35;drc=3fc49d4c4cd9e6202fe21f5925899292ffadb20a. This is also checked in the constructor so I changed the overflow checks to use `base::CheckedNumeric` instead.
`DCHECK_LT`? Ditto for `set()`.
Good catch! Fixed.
uint32_t length,Would it make sense to change these construction size args to `SafeHeapObjectSize` as well, so that the compiler flags attempts to downcast from 64-bit sizes here?
I think a downcast from 64-bit to 32-bit should be caught by the compiler warnings: https://source.chromium.org/chromium/chromium/src/+/main:v8/BUILD.gn;l=1870;drc=6e6da61a5be7bf1946bfcbdca494a8a6010d616b
I was thinking about using `SafeHeapObjectSize` wherever appropriate but I am not sure if we want this spill that into many parts of the codebase. All constructors check for the appropriate size and crash if it's over the limit so there should be no security impact for passing an invalid value here. In any case, I don't think it would be dangerous for the sandbox to allocate a larger array as long as we stay within the 32-bit bounds.
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 2 comments![]( "Open in Gerrit")
CHECK(!base::bits::SignedMulOverflow32(length, sizeof(T), &byte_length));Arash KazemiDo we keep the signed arithmetics here because we want the byte size to not exceed `kMaxInt`? In that case, please add a comment to document that intention.
The size of a FixedArray should be below kMaxInt: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/fixed-array.h;l=35;drc=3fc49d4c4cd9e6202fe21f5925899292ffadb20a. This is also checked in the constructor so I changed the overflow checks to use `base::CheckedNumeric` instead.
Consider running this against perf benchmarks (like jetstream) before landing.
uint32_t length,Arash KazemiWould it make sense to change these construction size args to `SafeHeapObjectSize` as well, so that the compiler flags attempts to downcast from 64-bit sizes here?
I think a downcast from 64-bit to 32-bit should be caught by the compiler warnings: https://source.chromium.org/chromium/chromium/src/+/main:v8/BUILD.gn;l=1870;drc=6e6da61a5be7bf1946bfcbdca494a8a6010d616b
I was thinking about using `SafeHeapObjectSize` wherever appropriate but I am not sure if we want this spill that into many parts of the codebase. All constructors check for the appropriate size and crash if it's over the limit so there should be no security impact for passing an invalid value here. In any case, I don't think it would be dangerous for the sandbox to allocate a larger array as long as we stay within the 32-bit bounds.
Acknowledged - if the existing warnings cover that then no need in adding the boilerplate to the code, agreed.
Related details
- Arash Kazemi
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Arash Kazemi
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
- string-unpack-code-SP.Average: base median = 1299.1266375545852 -> patched median = 1292.075356031312
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13f18ed2090000
Related details
- Arash Kazemi
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m1_mini_2020-perf/jetstream3 failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12e7ff1c090000
Related details
- Arash Kazemi
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream-main.crossbench complete.
- bigint-noble-ed25519: base median = 140.1879118707839 -> patched median = 139.8478804196705
- mandreel: base median = 175.34510698476385 -> patched median = 175.02503582194163
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10c07c74090000
Related details
- Arash Kazemi
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Arash Kazemi
| Commit-Queue | +1 |
2 comments
PTAL, added Andreas and Jakob for OWNERS in the regexp/wasm/compiler changes.
CHECK(!base::bits::SignedMulOverflow32(length, sizeof(T), &byte_length));Arash KazemiDo we keep the signed arithmetics here because we want the byte size to not exceed `kMaxInt`? In that case, please add a comment to document that intention.
Maksim IvanovThe size of a FixedArray should be below kMaxInt: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/fixed-array.h;l=35;drc=3fc49d4c4cd9e6202fe21f5925899292ffadb20a. This is also checked in the constructor so I changed the overflow checks to use `base::CheckedNumeric` instead.
Consider running this against perf benchmarks (like jetstream) before landing.
I ran the jetstream benchmarks and I don't see any impact. I also wouldn't expect one since I am not adding new CHECKs but changing existing ones.
Related details
- Andreas Haas
- Jakob Linke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreas Haas (Gerrit)
Andreas Haas voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andreas Haas
| Code-Review | +1 |
1 comment
const int lhs_length = lhs->length();Why don't you have the static cast here?
Related details
- Arash Kazemi
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi added 1 comment![]( "Open in Gerrit")
const int lhs_length = lhs->length();Why don't you have the static cast here?
`ZoneList::length` returns `int`, I added a `DCHECK` to ensure it's positive and then static cast it to `uint32_t`.
Related details
- Andreas Haas
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted and added 4 comments![]( "Open in Gerrit")
Votes added by Jakob Linke
| Code-Review | +1 |
4 comments
DCHECK_GT(ranges_length, 0);If we want to be extra clear about bounds, we'd have to protect against overflows from below, too.
I'm not saying we *should* do this; it's not clear to me that we gain any safety and it clutters the code. But explicit bounds checks seems to be what this CL series aims for, so up to you.
if (rhs->get(i * 2 + 0) != r.from()) return false;Same here.
range_array->set(i * 2 + 0, r.from());And here.
int mid, lower = 0, upper = static_cast<int>(ranges_len);Similarly, we could DCHECK that this is in int range.
Related details
- Andreas Haas
- Arash Kazemi
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi added 4 comments![]( "Open in Gerrit")
If we want to be extra clear about bounds, we'd have to protect against overflows from below, too.
I'm not saying we *should* do this; it's not clear to me that we gain any safety and it clutters the code. But explicit bounds checks seems to be what this CL series aims for, so up to you.
I added a DCHECK for the integer length which should cover all cases of overflow.
if (rhs->get(i * 2 + 0) != r.from()) return false;Same here.
I added `DCHECKs` for both get calls, `i` is less than `lhs_length` which is an int value so I don't think the first could actually trigger. If you think some of these are unnecessary or cluttering too much I am happy to remove them, they could be useful if the length is changed to be uint32_t in the future I guess.
range_array->set(i * 2 + 0, r.from());And here.
Same as above.
int mid, lower = 0, upper = static_cast<int>(ranges_len);Similarly, we could DCHECK that this is in int range.
- Andreas Haas
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jakob Linke
| Code-Review | +1 |
3 comments
if (rhs->get(i * 2 + 0) != r.from()) return false;Arash KazemiSame here.
I added `DCHECKs` for both get calls, `i` is less than `lhs_length` which is an int value so I don't think the first could actually trigger. If you think some of these are unnecessary or cluttering too much I am happy to remove them, they could be useful if the length is changed to be uint32_t in the future I guess.
Acknowledged
range_array->set(i * 2 + 0, r.from());Arash KazemiAnd here.
Same as above.
- Andreas Haas
- Arash Kazemi
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke added 5 comments![]( "Open in Gerrit")
for (uint32_t i = 0; i < counts_len / kBlockCountSlotSize; ++i) {These loop limits also have to be updated once below is fixed.
for (uint32_t i = 0; i < blocks_ids_len / kBlockIdSlotSize; ++i) {And here.
isolate, id_array_size_in_bytes, AllocationType::kOld);This appears to be wrong (existing bug): New takes the n of elements, not the byte size.
DirectHandle<FixedUInt32Array> counts = FixedUInt32Array::New(Same here.
return base::WriteUnalignedValue<T>(get_element_address(index), value);nit: could remove the `return`
Arash Kazemi (Gerrit)
Arash Kazemi added 5 comments![]( "Open in Gerrit")
for (uint32_t i = 0; i < counts_len / kBlockCountSlotSize; ++i) {These loop limits also have to be updated once below is fixed.
Done
for (uint32_t i = 0; i < blocks_ids_len / kBlockIdSlotSize; ++i) {Arash KazemiAnd here.
Done
This appears to be wrong (existing bug): New takes the n of elements, not the byte size.
Yes good catch, I fixed it to pass `blocks_count` and removed the `kBlockIdSlotSize`/`kBlockCountSlotSize` values since the loops don't need to skip 4bytes at a time now.
DirectHandle<FixedUInt32Array> counts = FixedUInt32Array::New(Arash KazemiSame here.
Done
return base::WriteUnalignedValue<T>(get_element_address(index), value);nit: could remove the `return`
Done
Related details
- Andreas Haas
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arash Kazemi
| Commit-Queue | +2 |
1 comment
Arash KazemiWhy don't you have the static cast here?
`ZoneList::length` returns `int`, I added a `DCHECK` to ensure it's positive and then static cast it to `uint32_t`.
Acknowledged
Related details
- Andreas Haas
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/diagnostics/basic-block-profiler.cc
Insertions: 7, Deletions: 16.
@@ -66,9 +66,6 @@
return isolate->factory()->NewStringFromAsciiChecked(source.c_str(),
AllocationType::kOld);
}
-
-constexpr uint32_t kBlockIdSlotSize = kInt32Size;
-constexpr uint32_t kBlockCountSlotSize = kInt32Size;
} // namespace
BasicBlockProfilerData::BasicBlockProfilerData(
@@ -90,12 +87,12 @@
Tagged<FixedUInt32Array> counts =
Cast<FixedUInt32Array>(js_heap_data->counts());
const uint32_t counts_len = counts->length().value();
- for (uint32_t i = 0; i < counts_len / kBlockCountSlotSize; ++i) {
+ for (uint32_t i = 0; i < counts_len; ++i) {
counts_.push_back(counts->get(i));
}
Tagged<FixedInt32Array> block_ids(js_heap_data->block_ids());
const uint32_t blocks_ids_len = block_ids->length().value();
- for (uint32_t i = 0; i < blocks_ids_len / kBlockIdSlotSize; ++i) {
+ for (uint32_t i = 0; i < blocks_ids_len; ++i) {
block_ids_.push_back(block_ids->get(i));
}
Tagged<PodArray<std::pair<int32_t, int32_t>>> branches =
@@ -111,20 +108,14 @@
DirectHandle<OnHeapBasicBlockProfilerData> BasicBlockProfilerData::CopyToJSHeap(
Isolate* isolate) {
const uint32_t blocks_count = static_cast<uint32_t>(n_blocks());
- uint32_t id_array_size_in_bytes = blocks_count * kBlockIdSlotSize;
- CHECK(static_cast<size_t>(id_array_size_in_bytes) / kBlockIdSlotSize ==
- blocks_count); // Overflow
- DirectHandle<FixedInt32Array> block_ids = FixedInt32Array::New(
- isolate, id_array_size_in_bytes, AllocationType::kOld);
+ DirectHandle<FixedInt32Array> block_ids =
+ FixedInt32Array::New(isolate, blocks_count, AllocationType::kOld);
for (uint32_t i = 0; i < blocks_count; ++i) {
block_ids->set(i, block_ids_[i]);
}
- uint32_t counts_array_size_in_bytes = blocks_count * kBlockCountSlotSize;
- CHECK(static_cast<size_t>(counts_array_size_in_bytes) / kBlockCountSlotSize ==
- blocks_count); // Overflow
- DirectHandle<FixedUInt32Array> counts = FixedUInt32Array::New(
- isolate, counts_array_size_in_bytes, AllocationType::kOld);
+ DirectHandle<FixedUInt32Array> counts =
+ FixedUInt32Array::New(isolate, blocks_count, AllocationType::kOld);
for (uint32_t i = 0; i < blocks_count; ++i) {
counts->set(i, counts_[i]);
}
@@ -156,7 +147,7 @@
DirectHandle<FixedUInt32Array> counts(
Cast<OnHeapBasicBlockProfilerData>(list->get(i))->counts(), isolate);
const uint32_t counts_len = counts->length().value();
- for (uint32_t j = 0; j < counts_len / kBlockCountSlotSize; ++j) {
+ for (uint32_t j = 0; j < counts_len; ++j) {
counts->set(j, 0);
}
}
```
```
The name of the file: src/objects/fixed-array-inl.h
Insertions: 1, Deletions: 1.
@@ -968,7 +968,7 @@
template <typename T, typename Base>
void FixedIntegerArrayBase<T, Base>::set(uint32_t index, T value) {
static_assert(std::is_integral_v<T>);
- return base::WriteUnalignedValue<T>(get_element_address(index), value);
+ base::WriteUnalignedValue<T>(get_element_address(index), value);
}
template <typename T, typename Base>
```
Change information
[sandbox] Use SafeHeapObjectSize in FixedIntegerArray and PodArray
This CL changes all methods in FixedIntegerArrayBase and PodArrayBase to
use uint32_t and return a strong alias for the length, forcing
conversion at the callsites.
- M src/compiler/backend/code-generator.cc
- M src/diagnostics/basic-block-profiler.cc
- M src/logging/log.cc
- M src/maglev/maglev-code-generator.cc
- M src/objects/fixed-array-inl.h
- M src/objects/fixed-array.h
- M src/regexp/regexp-macro-assembler.cc
- M src/wasm/wasm-objects-inl.h
- M src/wasm/wasm-objects.cc
- M src/wasm/wasm-objects.h
- M test/common/wasm/wasm-run-utils.cc
Code-Review: +1 by Maksim Ivanov, +1 by Andreas Haas, +1 by Jakob Linke
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |