In poking around in Phobos, I found a number of cases like:

    https://github.com/dlang/phobos/pull/4655

where overflow is possible in calculating storage sizes. Since allocation normally happens in @trusted code, these are a safety/security hole.

When reviewing Phobos submissions, please check for this sort of thing.

    https://wiki.dlang.org/Get_involved#Review_pull_requests

On 26.07.2016 00:17, Walter Bright wrote:
> In poking around in Phobos, I found a number of cases like:
>
>     https://github.com/dlang/phobos/pull/4655
>
> where overflow is possible in calculating storage sizes.  Since
> allocation normally happens in @trusted code, these are a
> safety/security hole.
> ...

According to the language documentation, the patch does not fix the problem.

https://dlang.org/spec/expression.html#AssertExpression

"The expression assert(0) is a special case; it signifies that it is unreachable code. [...] The optimization and code generation phases of compilation may assume that it is unreachable code."

One way the optimizer can use the assumption is for optimizing away the overflow check.

Your patch is just telling the optimizer that there is actually no security hole, even when that is not true. It is a bad idea to conflate assert and assume.

On Tuesday, 26 July 2016 at 14:28:48 UTC, Timon Gehr wrote:
> "The expression assert(0) is a special case; it signifies that it is unreachable code. [...] The optimization and code generation phases of compilation may assume that it is unreachable code."

so specs should be fixed. i bet everyone is using `assert(0);` to mark some "immediate error exit" cases.

A perfect example for an item for your action list.

On Tuesday, 26 July 2016 at 14:28:48 UTC, Timon Gehr wrote:
>
> According to the language documentation, the patch does not fix the problem.
>
> https://dlang.org/spec/expression.html#AssertExpression
>
> "The expression assert(0) is a special case; it signifies that it is unreachable code. [...] The optimization and code generation phases of compilation may assume that it is unreachable code."
>
> One way the optimizer can use the assumption is for optimizing away the overflow check.

This is not true. The spec is unclear, but what makes assert(0) special is that the compiler is not allowed to remove it from the program, not at any optimization level (other asserts can and will be removed by the compiler, see `-release`).
The compiler can assume it is unreachable code, but it has to keep it, so:
```
if (foo()) {
  // compiler can assume _almost_ zero probability for taking this branch
  assert(0);
}
```

On 26.07.2016 17:44, Johan Engelen wrote:
> On Tuesday, 26 July 2016 at 14:28:48 UTC, Timon Gehr wrote:
>>
>> According to the language documentation, the patch does not fix the
>> problem.
>>
>> https://dlang.org/spec/expression.html#AssertExpression
>>
>> "The expression assert(0) is a special case; it signifies that it is
>> unreachable code. [...] The optimization and code generation phases of
>> compilation may assume that it is unreachable code."
>>
>> One way the optimizer can use the assumption is for optimizing away
>> the overflow check.
>
> This is not true. The spec is unclear,

The spec claims that this is true. If it isn't, the spec is in error.

> but what makes assert(0) special
> is that the compiler is not allowed to remove it from the program, not
> at any optimization level (other asserts can and will be removed by the
> compiler, see `-release`).

I would assume that the compiler is allowed to remove it from the program if it can prove it is unreachable.

> The compiler can assume it is unreachable code, but it has to keep it,

That makes no sense. Those two statements are mutually exclusive.

> so:
> ```
> if (foo()) {
>   // compiler can assume _almost_ zero probability for taking this branch
>   assert(0);
> }
> ```

That makes more sense.

On 7/26/16 2:44 PM, Timon Gehr wrote:
> On 26.07.2016 17:44, Johan Engelen wrote:
>> The compiler can assume it is unreachable code, but it has to keep it,
>
> That makes no sense. Those two statements are mutually exclusive.

I thought that assert(0) means that the compiler does not need to check any validity of code after the assert. I'd reword Johan's statement to say that the compiler can assume the programmer thought this was unreachable, so it can just crash. It can't compile the crash out, or then the program is in an invalid state!

This is not the user saying "I guarantee we won't go over the cliff", it's the user saying "If we get to this point, I have lost all guarantees of not going off the cliff, so shut off the engine". The compiler can make optimization assumptions for the code that *doesn't* take the branch, but it still needs the branch to make sure.

-Steve

On 7/26/2016 8:24 AM, Robert burner Schadek wrote:
> A perfect example for an item for your action list.

Anybody can work on this!

On Tuesday, 26 July 2016 at 21:53:48 UTC, Walter Bright wrote:
> On 7/26/2016 8:24 AM, Robert burner Schadek wrote:
>> A perfect example for an item for your action list.
>
> Anybody can work on this!

That's the point! Put it on the list so people know.

On 7/26/2016 7:28 AM, Timon Gehr wrote:
> According to the language documentation, the patch does not fix the problem.
>
> https://dlang.org/spec/expression.html#AssertExpression
>
> "The expression assert(0) is a special case; it signifies that it is unreachable
> code. [...] The optimization and code generation phases of compilation may
> assume that it is unreachable code."
>
> One way the optimizer can use the assumption is for optimizing away the overflow
> check.
>
> Your patch is just telling the optimizer that there is actually no security
> hole, even when that is not true. It is a bad idea to conflate assert and assume.

What the assert(0) actually does is insert a HALT instruction, even when -release is used. The spec is poorly worded.