Skip to content

Commit 0e4f9a7

Browse files
borsbors
committed
Auto merge of rust-lang#95295 - CAD97:layout-isize, r=scottmcm
Enforce that layout size fits in isize in Layout As it turns out, enforcing this _in APIs that already enforce `usize` overflow_ is fairly trivial. `Layout::from_size_align_unchecked` continues to "allow" sizes which (when rounded up) would overflow `isize`, but these are now declared as library UB for `Layout`, meaning that consumers of `Layout` no longer have to check this before making an allocation. (Note that this is "immediate library UB;" IOW it is valid for a future release to make this immediate "language UB," and there is an extant patch to do so, to allow Miri to catch this misuse.) See also rust-lang#95252, [Zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Layout.20Isn't.20Enforcing.20The.20isize.3A.3AMAX.20Rule). Fixes rust-lang#95334 Some relevant quotes: `@eddyb,` rust-lang#95252 (comment) > [B]ecause of the non-trivial presence of both of these among code published on e.g. crates.io: > > 1. **`Layout` "producers" / `GlobalAlloc` "users"**: smart pointers (including `alloc::rc` copies with small tweaks), collections, etc. > 2. **`Layout` "consumers" / `GlobalAlloc` "providers"**: perhaps fewer of these, but anything built on top of OS APIs like `mmap` will expose `> isize::MAX` allocations (on 32-bit hosts) if they lack extra checks > > IMO the only responsible option is to enforce the `isize::MAX` limit in `Layout`, which: > > * makes `Layout` _sound_ in terms of only ever allowing allocations where `(alloc_base_ptr: *mut u8).offset(size)` is never UB > * frees both "producers" and "consumers" of `Layout` from manually reimplementing the checks > * manual checks can be risky, e.g. if the final size passed to the allocator isn't the one being checked > * this applies retroactively, fixing the overall soundness of existing code with zero transition period or _any_ changes required from users (as long as going through `Layout` is mandatory, making a "choke point") > > > Feel free to quote this comment onto any relevant issue, I might not be able to keep track of developments. `@Gankra,` rust-lang#95252 (comment) > As someone who spent way too much time optimizing libcollections checks for this stuff and tried to splatter docs about it everywhere on the belief that it was a reasonable thing for people to manually take care of: I concede the point, it is not reasonable. I am wholy spiritually defeated by the fact that _liballoc_ of all places is getting this stuff wrong. This isn't throwing shade at the folks who implemented these Rc features, but rather a statement of how impractical it is to expect anyone out in the wider ecosystem to enforce them if _some of the most audited rust code in the library that defines the very notion of allocating memory_ can't even reliably do it. > > We need the nuclear option of Layout enforcing this rule. Code that breaks this rule is _deeply_ broken and any "regressions" from changing Layout's contract is a _correctness_ fix. Anyone who disagrees and is sufficiently motivated can go around our backs but the standard library should 100% refuse to enable them. cc also `@RalfJung` `@rust-lang/wg-allocators.` Even though this technically supersedes rust-lang#95252, those potential failure points should almost certainly still get nicer panics than just "unwrap failed" (which they would get by this PR). It might additionally be worth recommending to users of the `Layout` API that they should ideally use `.and_then`/`?` to complete the entire layout calculation, and then `panic!` from a single location at the end of `Layout` manipulation, to reduce the overhead of the checks and optimizations preserving the exact location of each `panic` which are conceptually just one failure: allocation too big. Probably deserves a T-lang and/or T-libs-api FCP (this technically solidifies the [objects must be no larger than `isize::MAX`](https://rust-lang.github.io/unsafe-code-guidelines/layout/scalars.html#isize-and-usize) rule further, and the UCG document says this hasn't been RFCd) and a crater run. Ideally, no code exists that will start failing with this addition; if it does, it was _likely_ (but not certainly) causing UB. Changes the raw_vec allocation path, thus deserves a perf run as well. I suggest hiding whitespace-only changes in the diff view.
2 parents 41e7378 + 6f2af2a commit 0e4f9a7

File tree

4 files changed

+156
-331
lines changed

4 files changed

+156
-331
lines changed

‎alloc/tests/string.rs

+38-88
Original file line numberDiff line numberDiff line change
@@ -693,12 +693,6 @@ fn test_try_reserve() {
693693
const MAX_CAP: usize = isize::MAX as usize;
694694
const MAX_USIZE: usize = usize::MAX;
695695

696-
// On 16/32-bit, we check that allocations don't exceed isize::MAX,
697-
// on 64-bit, we assume the OS will give an OOM for such a ridiculous size.
698-
// Any platform that succeeds for these requests is technically broken with
699-
// ptr::offset because LLVM is the worst.
700-
let guards_against_isize = usize::BITS < 64;
701-
702696
{
703697
// Note: basic stuff is checked by test_reserve
704698
let mut empty_string: String = String::new();
@@ -712,35 +706,19 @@ fn test_try_reserve() {
712706
panic!("isize::MAX shouldn't trigger an overflow!");
713707
}
714708

715-
if guards_against_isize {
716-
// Check isize::MAX + 1 does count as overflow
717-
assert_matches!(
718-
empty_string.try_reserve(MAX_CAP + 1).map_err(|e| e.kind()),
719-
Err(CapacityOverflow),
720-
"isize::MAX + 1 should trigger an overflow!"
721-
);
722-
723-
// Check usize::MAX does count as overflow
724-
assert_matches!(
725-
empty_string.try_reserve(MAX_USIZE).map_err(|e| e.kind()),
726-
Err(CapacityOverflow),
727-
"usize::MAX should trigger an overflow!"
728-
);
729-
} else {
730-
// Check isize::MAX + 1 is an OOM
731-
assert_matches!(
732-
empty_string.try_reserve(MAX_CAP + 1).map_err(|e| e.kind()),
733-
Err(AllocError { .. }),
734-
"isize::MAX + 1 should trigger an OOM!"
735-
);
736-
737-
// Check usize::MAX is an OOM
738-
assert_matches!(
739-
empty_string.try_reserve(MAX_USIZE).map_err(|e| e.kind()),
740-
Err(AllocError { .. }),
741-
"usize::MAX should trigger an OOM!"
742-
);
743-
}
709+
// Check isize::MAX + 1 does count as overflow
710+
assert_matches!(
711+
empty_string.try_reserve(MAX_CAP + 1).map_err(|e| e.kind()),
712+
Err(CapacityOverflow),
713+
"isize::MAX + 1 should trigger an overflow!"
714+
);
715+
716+
// Check usize::MAX does count as overflow
717+
assert_matches!(
718+
empty_string.try_reserve(MAX_USIZE).map_err(|e| e.kind()),
719+
Err(CapacityOverflow),
720+
"usize::MAX should trigger an overflow!"
721+
);
744722
}
745723

746724
{
@@ -753,19 +731,13 @@ fn test_try_reserve() {
753731
if let Err(CapacityOverflow) = ten_bytes.try_reserve(MAX_CAP - 10).map_err(|e| e.kind()) {
754732
panic!("isize::MAX shouldn't trigger an overflow!");
755733
}
756-
if guards_against_isize {
757-
assert_matches!(
758-
ten_bytes.try_reserve(MAX_CAP - 9).map_err(|e| e.kind()),
759-
Err(CapacityOverflow),
760-
"isize::MAX + 1 should trigger an overflow!"
761-
);
762-
} else {
763-
assert_matches!(
764-
ten_bytes.try_reserve(MAX_CAP - 9).map_err(|e| e.kind()),
765-
Err(AllocError { .. }),
766-
"isize::MAX + 1 should trigger an OOM!"
767-
);
768-
}
734+
735+
assert_matches!(
736+
ten_bytes.try_reserve(MAX_CAP - 9).map_err(|e| e.kind()),
737+
Err(CapacityOverflow),
738+
"isize::MAX + 1 should trigger an overflow!"
739+
);
740+
769741
// Should always overflow in the add-to-len
770742
assert_matches!(
771743
ten_bytes.try_reserve(MAX_USIZE).map_err(|e| e.kind()),
@@ -785,8 +757,6 @@ fn test_try_reserve_exact() {
785757
const MAX_CAP: usize = isize::MAX as usize;
786758
const MAX_USIZE: usize = usize::MAX;
787759

788-
let guards_against_isize = usize::BITS < 64;
789-
790760
{
791761
let mut empty_string: String = String::new();
792762

@@ -799,31 +769,17 @@ fn test_try_reserve_exact() {
799769
panic!("isize::MAX shouldn't trigger an overflow!");
800770
}
801771

802-
if guards_against_isize {
803-
assert_matches!(
804-
empty_string.try_reserve_exact(MAX_CAP + 1).map_err(|e| e.kind()),
805-
Err(CapacityOverflow),
806-
"isize::MAX + 1 should trigger an overflow!"
807-
);
808-
809-
assert_matches!(
810-
empty_string.try_reserve_exact(MAX_USIZE).map_err(|e| e.kind()),
811-
Err(CapacityOverflow),
812-
"usize::MAX should trigger an overflow!"
813-
);
814-
} else {
815-
assert_matches!(
816-
empty_string.try_reserve_exact(MAX_CAP + 1).map_err(|e| e.kind()),
817-
Err(AllocError { .. }),
818-
"isize::MAX + 1 should trigger an OOM!"
819-
);
820-
821-
assert_matches!(
822-
empty_string.try_reserve_exact(MAX_USIZE).map_err(|e| e.kind()),
823-
Err(AllocError { .. }),
824-
"usize::MAX should trigger an OOM!"
825-
);
826-
}
772+
assert_matches!(
773+
empty_string.try_reserve_exact(MAX_CAP + 1).map_err(|e| e.kind()),
774+
Err(CapacityOverflow),
775+
"isize::MAX + 1 should trigger an overflow!"
776+
);
777+
778+
assert_matches!(
779+
empty_string.try_reserve_exact(MAX_USIZE).map_err(|e| e.kind()),
780+
Err(CapacityOverflow),
781+
"usize::MAX should trigger an overflow!"
782+
);
827783
}
828784

829785
{
@@ -839,19 +795,13 @@ fn test_try_reserve_exact() {
839795
{
840796
panic!("isize::MAX shouldn't trigger an overflow!");
841797
}
842-
if guards_against_isize {
843-
assert_matches!(
844-
ten_bytes.try_reserve_exact(MAX_CAP - 9).map_err(|e| e.kind()),
845-
Err(CapacityOverflow),
846-
"isize::MAX + 1 should trigger an overflow!"
847-
);
848-
} else {
849-
assert_matches!(
850-
ten_bytes.try_reserve_exact(MAX_CAP - 9).map_err(|e| e.kind()),
851-
Err(AllocError { .. }),
852-
"isize::MAX + 1 should trigger an OOM!"
853-
);
854-
}
798+
799+
assert_matches!(
800+
ten_bytes.try_reserve_exact(MAX_CAP - 9).map_err(|e| e.kind()),
801+
Err(CapacityOverflow),
802+
"isize::MAX + 1 should trigger an overflow!"
803+
);
804+
855805
assert_matches!(
856806
ten_bytes.try_reserve_exact(MAX_USIZE).map_err(|e| e.kind()),
857807
Err(CapacityOverflow),

‎alloc/tests/vec.rs

+52-114
Original file line numberDiff line numberDiff line change
@@ -1517,12 +1517,6 @@ fn test_try_reserve() {
15171517
const MAX_CAP: usize = isize::MAX as usize;
15181518
const MAX_USIZE: usize = usize::MAX;
15191519

1520-
// On 16/32-bit, we check that allocations don't exceed isize::MAX,
1521-
// on 64-bit, we assume the OS will give an OOM for such a ridiculous size.
1522-
// Any platform that succeeds for these requests is technically broken with
1523-
// ptr::offset because LLVM is the worst.
1524-
let guards_against_isize = usize::BITS < 64;
1525-
15261520
{
15271521
// Note: basic stuff is checked by test_reserve
15281522
let mut empty_bytes: Vec<u8> = Vec::new();
@@ -1536,35 +1530,19 @@ fn test_try_reserve() {
15361530
panic!("isize::MAX shouldn't trigger an overflow!");
15371531
}
15381532

1539-
if guards_against_isize {
1540-
// Check isize::MAX + 1 does count as overflow
1541-
assert_matches!(
1542-
empty_bytes.try_reserve(MAX_CAP + 1).map_err(|e| e.kind()),
1543-
Err(CapacityOverflow),
1544-
"isize::MAX + 1 should trigger an overflow!"
1545-
);
1546-
1547-
// Check usize::MAX does count as overflow
1548-
assert_matches!(
1549-
empty_bytes.try_reserve(MAX_USIZE).map_err(|e| e.kind()),
1550-
Err(CapacityOverflow),
1551-
"usize::MAX should trigger an overflow!"
1552-
);
1553-
} else {
1554-
// Check isize::MAX + 1 is an OOM
1555-
assert_matches!(
1556-
empty_bytes.try_reserve(MAX_CAP + 1).map_err(|e| e.kind()),
1557-
Err(AllocError { .. }),
1558-
"isize::MAX + 1 should trigger an OOM!"
1559-
);
1560-
1561-
// Check usize::MAX is an OOM
1562-
assert_matches!(
1563-
empty_bytes.try_reserve(MAX_USIZE).map_err(|e| e.kind()),
1564-
Err(AllocError { .. }),
1565-
"usize::MAX should trigger an OOM!"
1566-
);
1567-
}
1533+
// Check isize::MAX + 1 does count as overflow
1534+
assert_matches!(
1535+
empty_bytes.try_reserve(MAX_CAP + 1).map_err(|e| e.kind()),
1536+
Err(CapacityOverflow),
1537+
"isize::MAX + 1 should trigger an overflow!"
1538+
);
1539+
1540+
// Check usize::MAX does count as overflow
1541+
assert_matches!(
1542+
empty_bytes.try_reserve(MAX_USIZE).map_err(|e| e.kind()),
1543+
Err(CapacityOverflow),
1544+
"usize::MAX should trigger an overflow!"
1545+
);
15681546
}
15691547

15701548
{
@@ -1577,19 +1555,13 @@ fn test_try_reserve() {
15771555
if let Err(CapacityOverflow) = ten_bytes.try_reserve(MAX_CAP - 10).map_err(|e| e.kind()) {
15781556
panic!("isize::MAX shouldn't trigger an overflow!");
15791557
}
1580-
if guards_against_isize {
1581-
assert_matches!(
1582-
ten_bytes.try_reserve(MAX_CAP - 9).map_err(|e| e.kind()),
1583-
Err(CapacityOverflow),
1584-
"isize::MAX + 1 should trigger an overflow!"
1585-
);
1586-
} else {
1587-
assert_matches!(
1588-
ten_bytes.try_reserve(MAX_CAP - 9).map_err(|e| e.kind()),
1589-
Err(AllocError { .. }),
1590-
"isize::MAX + 1 should trigger an OOM!"
1591-
);
1592-
}
1558+
1559+
assert_matches!(
1560+
ten_bytes.try_reserve(MAX_CAP - 9).map_err(|e| e.kind()),
1561+
Err(CapacityOverflow),
1562+
"isize::MAX + 1 should trigger an overflow!"
1563+
);
1564+
15931565
// Should always overflow in the add-to-len
15941566
assert_matches!(
15951567
ten_bytes.try_reserve(MAX_USIZE).map_err(|e| e.kind()),
@@ -1610,19 +1582,13 @@ fn test_try_reserve() {
16101582
{
16111583
panic!("isize::MAX shouldn't trigger an overflow!");
16121584
}
1613-
if guards_against_isize {
1614-
assert_matches!(
1615-
ten_u32s.try_reserve(MAX_CAP / 4 - 9).map_err(|e| e.kind()),
1616-
Err(CapacityOverflow),
1617-
"isize::MAX + 1 should trigger an overflow!"
1618-
);
1619-
} else {
1620-
assert_matches!(
1621-
ten_u32s.try_reserve(MAX_CAP / 4 - 9).map_err(|e| e.kind()),
1622-
Err(AllocError { .. }),
1623-
"isize::MAX + 1 should trigger an OOM!"
1624-
);
1625-
}
1585+
1586+
assert_matches!(
1587+
ten_u32s.try_reserve(MAX_CAP / 4 - 9).map_err(|e| e.kind()),
1588+
Err(CapacityOverflow),
1589+
"isize::MAX + 1 should trigger an overflow!"
1590+
);
1591+
16261592
// Should fail in the mul-by-size
16271593
assert_matches!(
16281594
ten_u32s.try_reserve(MAX_USIZE - 20).map_err(|e| e.kind()),
@@ -1642,8 +1608,6 @@ fn test_try_reserve_exact() {
16421608
const MAX_CAP: usize = isize::MAX as usize;
16431609
const MAX_USIZE: usize = usize::MAX;
16441610

1645-
let guards_against_isize = size_of::<usize>() < 8;
1646-
16471611
{
16481612
let mut empty_bytes: Vec<u8> = Vec::new();
16491613

@@ -1656,31 +1620,17 @@ fn test_try_reserve_exact() {
16561620
panic!("isize::MAX shouldn't trigger an overflow!");
16571621
}
16581622

1659-
if guards_against_isize {
1660-
assert_matches!(
1661-
empty_bytes.try_reserve_exact(MAX_CAP + 1).map_err(|e| e.kind()),
1662-
Err(CapacityOverflow),
1663-
"isize::MAX + 1 should trigger an overflow!"
1664-
);
1665-
1666-
assert_matches!(
1667-
empty_bytes.try_reserve_exact(MAX_USIZE).map_err(|e| e.kind()),
1668-
Err(CapacityOverflow),
1669-
"usize::MAX should trigger an overflow!"
1670-
);
1671-
} else {
1672-
assert_matches!(
1673-
empty_bytes.try_reserve_exact(MAX_CAP + 1).map_err(|e| e.kind()),
1674-
Err(AllocError { .. }),
1675-
"isize::MAX + 1 should trigger an OOM!"
1676-
);
1677-
1678-
assert_matches!(
1679-
empty_bytes.try_reserve_exact(MAX_USIZE).map_err(|e| e.kind()),
1680-
Err(AllocError { .. }),
1681-
"usize::MAX should trigger an OOM!"
1682-
);
1683-
}
1623+
assert_matches!(
1624+
empty_bytes.try_reserve_exact(MAX_CAP + 1).map_err(|e| e.kind()),
1625+
Err(CapacityOverflow),
1626+
"isize::MAX + 1 should trigger an overflow!"
1627+
);
1628+
1629+
assert_matches!(
1630+
empty_bytes.try_reserve_exact(MAX_USIZE).map_err(|e| e.kind()),
1631+
Err(CapacityOverflow),
1632+
"usize::MAX should trigger an overflow!"
1633+
);
16841634
}
16851635

16861636
{
@@ -1696,19 +1646,13 @@ fn test_try_reserve_exact() {
16961646
{
16971647
panic!("isize::MAX shouldn't trigger an overflow!");
16981648
}
1699-
if guards_against_isize {
1700-
assert_matches!(
1701-
ten_bytes.try_reserve_exact(MAX_CAP - 9).map_err(|e| e.kind()),
1702-
Err(CapacityOverflow),
1703-
"isize::MAX + 1 should trigger an overflow!"
1704-
);
1705-
} else {
1706-
assert_matches!(
1707-
ten_bytes.try_reserve_exact(MAX_CAP - 9).map_err(|e| e.kind()),
1708-
Err(AllocError { .. }),
1709-
"isize::MAX + 1 should trigger an OOM!"
1710-
);
1711-
}
1649+
1650+
assert_matches!(
1651+
ten_bytes.try_reserve_exact(MAX_CAP - 9).map_err(|e| e.kind()),
1652+
Err(CapacityOverflow),
1653+
"isize::MAX + 1 should trigger an overflow!"
1654+
);
1655+
17121656
assert_matches!(
17131657
ten_bytes.try_reserve_exact(MAX_USIZE).map_err(|e| e.kind()),
17141658
Err(CapacityOverflow),
@@ -1729,19 +1673,13 @@ fn test_try_reserve_exact() {
17291673
{
17301674
panic!("isize::MAX shouldn't trigger an overflow!");
17311675
}
1732-
if guards_against_isize {
1733-
assert_matches!(
1734-
ten_u32s.try_reserve_exact(MAX_CAP / 4 - 9).map_err(|e| e.kind()),
1735-
Err(CapacityOverflow),
1736-
"isize::MAX + 1 should trigger an overflow!"
1737-
);
1738-
} else {
1739-
assert_matches!(
1740-
ten_u32s.try_reserve_exact(MAX_CAP / 4 - 9).map_err(|e| e.kind()),
1741-
Err(AllocError { .. }),
1742-
"isize::MAX + 1 should trigger an OOM!"
1743-
);
1744-
}
1676+
1677+
assert_matches!(
1678+
ten_u32s.try_reserve_exact(MAX_CAP / 4 - 9).map_err(|e| e.kind()),
1679+
Err(CapacityOverflow),
1680+
"isize::MAX + 1 should trigger an overflow!"
1681+
);
1682+
17451683
assert_matches!(
17461684
ten_u32s.try_reserve_exact(MAX_USIZE - 20).map_err(|e| e.kind()),
17471685
Err(CapacityOverflow),

0 commit comments

Comments
 (0)