file.d: add overflow checks

[WalterBright]

No description provided.

[MartinNowak]

Why?
You'll always crash before reaching that overflow. It's a size_t, so your address space is gone before it overflows.
That's too much ugly code for it's uselessness.

[WalterBright]

You'll always crash before reaching that overflow.

Famous last words :-)

All assert's are guaranteed to never be tripped. Of course, we're often wrong about that, and they get tripped. Hence adding this here.

[MartinNowak]

See, this is the mess w/ all those crappy tiny PRs.
Now we having plenty of separate discussions #4712, #4713, #4714, #4715, #4716, #4717, #4718 about the same thing. This literally feels like sneaking in changes to me.

[MartinNowak]

Where does the sudden interest in size_t overflows come from? In particular when used in buffer resizing there seems to be no practical use, b/c as pointed out above, you'll always end up w/o address space long before the overflow.

[MartinNowak]

Before flooding phobos w/ even more of this ugly code, please write a small SafeInt helper in std/internal that does those checks.

[schveiguy]

you'll always end up w/o address space long before the overflow.

Not necessarily. In many cases, you request a malloc for the result of a multiplication. If it overflows, the size actually granted could succeed and be quite small. But because malloc only tells you that you succeeded and here's your pointer, it's up to you to make that into a safe D array. In other words, your code thinks you requested space for 2 billion int, but instead you got space for 50 million. However, you just happily slice the pointer for 2 billion having a large unsafe access to all address space. In 64 bits, the overflow case is much less likely.

For the addition overflows, I agree the benefit is debatable. But if you are going to check for one case, you may as well check for all.

I do agree that a SafeInt type would look MUCH better than all these calls. It's easy to miss some operations too without such a nice wrapper type.

[JackStouffer]

I do agree that a SafeInt type would look MUCH better than all these calls. It's easy to miss some operations too without such a nice wrapper type.

Then review #4613

[andralex]

This is getting to the point where Checked is justified. Simply replacing size_t size = 0; with auto size = checked!size_t(0); and leaving all other code unchanged takes care of it.

[JackStouffer]

@andralex I thought the current rule was "no std.experimental use in std".

[andralex]

@JackStouffer on the contrary, use of experimental facilities in phobos is good dogfooding.

[dnadlinger]

@andralex @JackStouffer: Agreed. std.experimental shouldn't leak into the interface, but use in the implementation is fine. If the worst comes to the worst, we could always keep a private copy of the necessary parts in std.internal.

[WalterBright]

Simply replacing size_t size = 0; with auto size = checked!size_t(0); and leaving all other code unchanged takes care of it.

Turns out it's more than that, as the diffs show.

[WalterBright]

Hmm, now I get:

object.Error@src/rt/minfo.d(371): Cyclic dependency between module std.experimental.allocator and std.concurrency
std.experimental.allocator* ->
std.conv ->
std.datetime ->
std.datetime.timezone ->
std.concurrency* ->
std.stdio ->
std.file ->
std.experimental.checkedint ->
std.experimental.allocator.common ->
std.experimental.allocator*

checkedint should not be having cycles

[andralex]

Time to get rid of them static shared this(). #5421

[andralex]

#5423, too

[andralex]

Couple of things that make the code a tad simpler. In fact I'll try to make the changes myself.

[andralex]

Abort is the default, please use checked(size_t(0))

[andralex]

neat idea

[andralex]

checked(n)

[andralex]

@WalterBright I couldn't edit this PR. Please rebase and mind the review - thx!

[JackStouffer]

As this is dead in the water, I've revived the changes here: #5990