Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Add some more overflow checks inside array allocation #1675

Merged
merged 3 commits into from Feb 18, 2017
Merged

Add some more overflow checks inside array allocation #1675

merged 3 commits into from Feb 18, 2017

Conversation

Burgos
Copy link
Member

@Burgos Burgos commented Oct 19, 2016

ntrel
ntrel reviewed Feb 10, 2017
View reviewed changes
src/rt/lifetime.d Outdated
@@ -274,7 +274,8 @@ bool __setArrayAllocLength(ref BlkInfo info, size_t newlength, bool isshared, co

if(info.size <= 256)
{
if(newlength + SMALLPAD + typeInfoSize > info.size)
auto newlength_padded = newlength + SMALLPAD + typeInfoSize;
if(newlength + SMALLPAD + typeInfoSize > info.size || newlength_padded < newlength)
Copy link
Contributor

Choose a reason for hiding this comment

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

DRY: if (newlength_padded > info.size ||

@dnadlinger
Copy link
Member

dnadlinger commented Feb 10, 2017 •
edited

I'd probably use core.checkedint.addu to make the intent explicit, even though that API is a little clunky for single use.

dgileadi
dgileadi reviewed Feb 10, 2017
View reviewed changes
test/allocations/src/overflow_from_zero.d Outdated
{
void[] buffer;
buffer.length = 1;
buffer.length = size_t.max;
Copy link

Choose a reason for hiding this comment

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

This file seems to have exactly the same content as overflow_from_existing.d. Is that on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I completely missed it!

@WalterBright
Copy link
Member

Yes, please use core.checkedint. This kind of thing is exactly what it is for. It's been documented by Regher that getting overflow checking right can be surprisingly tricky, and most programmers get it wrong. Hence, the motivation for core.checkedint where it only has to be gotten right once, instead of every time. It'll also save on the grief of anyone reviewing the ad-hoc implementations to make sure they are correct which, again, is very tricky.

@Burgos
Copy link
Member Author

Burgos commented Feb 11, 2017

Well, this is all nice to see on a PR :-)

I'll check and and use the checkedint, if not today, then over the weekend.

@Burgos
Fix issue 16470: Segfault with negative array length
2d38937 
If the array length is too large, and the element
size small enough, the overflow might go undetected
in the check while multiplying array size and element
size, but it can later manifest when adding padding, etc.

https://issues.dlang.org/show_bug.cgi?id=16470
@Burgos
Check if the array allocation succeded
0de554a
@Burgos
Add tests for "Out of memory exception"
f1ce9d8
@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
✓ 16470 Segfault with negative array length

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Feb 12, 2017
@Burgos
Copy link
Member Author

Burgos commented Feb 12, 2017

Updated to use core.checkedint, and test-case fix.

@Burgos
Copy link
Member Author

Burgos commented Feb 12, 2017

And commit title to trigger bugzilla integration.

@Burgos
Copy link
Member Author

Burgos commented Feb 14, 2017

Friendly ping :-)

@WalterBright
Copy link
Member

Auto-merge toggled on

@WalterBright WalterBright merged commit 3485ff8 into dlang:master Feb 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dgileadi dgileadi dgileadi left review comments

@ntrel ntrel ntrel left review comments

Assignees
No one assigned
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
Milestone
No milestone
6 participants
@Burgos @dnadlinger @WalterBright @dlang-bot @dgileadi @ntrel