https://issues.dlang.org/show_bug.cgi?id=19916

Nicholas Wilson <iamthewilsonator@hotmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |iamthewilsonator@hotmail.co
                   |                            |m

--- Comment #1 from Nicholas Wilson <iamthewilsonator@hotmail.com> ---
This is already the case of the union contains pointers.

--

https://issues.dlang.org/show_bug.cgi?id=19916

--- Comment #2 from Manu <turkeyman@gmail.com> ---
I have seen such a complaint appear occasionally. I don't know what motivates it, but the truth of the matter is, literally any access to any member of a union is unsafe.

--

https://issues.dlang.org/show_bug.cgi?id=19916

Simen Kjaeraas <simen.kjaras@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simen.kjaras@gmail.com

--- Comment #3 from Simen Kjaeraas <simen.kjaras@gmail.com> ---
@safe deals with memory safety. Can you show a case where a union containing no pointers or references causes memory safety issues?

--

https://issues.dlang.org/show_bug.cgi?id=19916

--- Comment #4 from Manu <turkeyman@gmail.com> ---
union U
{
  int x = 10;
  float f = void;
  NotAPointer np = void;
}
U u;

float r = 10.0 + u.f; // <- f is uninitialised, r is junk

np.callMethod(); // <- np is not a pointer, but still uninitialised, certain
crash

etc.

Any access to a union member may interact with uninitialised memory. Accessing uninitialised memory is a memory safety issue.

--

https://issues.dlang.org/show_bug.cgi?id=19916

--- Comment #5 from Simen Kjaeraas <simen.kjaras@gmail.com> ---
(In reply to Manu from comment #4)
> union U
> {
>   int x = 10;
>   float f = void;
>   NotAPointer np = void;
> }
> U u;
> 
> float r = 10.0 + u.f; // <- f is uninitialised, r is junk

This is not a memory safety issue.


> u.np.callMethod(); // <- np is not a pointer, but still uninitialised, certain
> crash
> 
> Any access to a union member may interact with uninitialised memory. Accessing uninitialised memory is a memory safety issue.

I think I found an example that qualifies:

module someothermodule;

struct NotAPointer {
    private size_t n;
    @disable this();
    @trusted this(int* p) {
        assert(p.isValid);
        n = cast(size_t)p;
    }
    @trusted void callMethod() {
        *cast(int*)n = 3;
    }
}

While NotAPointer's wanton casting looks suspicious, p.isValid ensures the pointer will stay valid as long as NotAPointer exists. We could even have a private constructor and force the use of a factory function to ensure no stupid pointers are passed to the constructor. Also, the use of @disable this() and the private member ensures @safe user code won't be able to set n to something silly.

...except for unions and void initialization. Both of these wreak havoc with the assumptions of NotAPointer.

So yeah, I concede - there are clear ways to cause memory safety issues by simple union member access in @safe methods.

--

https://issues.dlang.org/show_bug.cgi?id=19916

--- Comment #6 from Manu <turkeyman@gmail.com> ---
Glad you agree.

I think it's overwhelmingly simple; if the compiler can't confirm that the memory it's accessing is valid, then it's not @safe, and if that's not the definition of @safe, then we need to fix the definition.

Accessing uninitialised memory is absolutely a memory safety issue. I don't know where this idea that it has strictly to do with pointers comes from? Why would safety be limited that way?

Anyway, let's fix this.

--

https://issues.dlang.org/show_bug.cgi?id=19916

Dennis <dkorpel@live.nl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dkorpel@live.nl

--- Comment #7 from Dennis <dkorpel@live.nl> ---
(In reply to Manu from comment #6)
> Accessing uninitialised memory is absolutely a memory safety issue.

Not per se. This compiles, prints a random number, and doesn't corrupt memory.

```
import std;

void main() @safe
{
    int a = void;
    writeln(a);
}
```

> I don't know where this idea that it has strictly to do with pointers comes from? Why would safety be limited that way?

Paraphrasing Walter from his DConf 2017 keynote, memory safety is not about 'no memory related bugs', it's "a concern in software development that aims to avoid software bugs that cause security vulnerabilities dealing with random-access memory (RAM) access, such as buffer overflows and dangling pointers". Uninitialized / overlapped pointers can cause such issues, uninitialized integers can not.

Disallowing a simple harmless sum-type in @safe invites more use of @trusted giving more opportunities for actual memory corrupting bugs to creep in. Not to mention it would break existing code.

Unless there is a way to actually corrupt memory in @safe code using this (without using @trusted) it's not something @safe should prevent.

--

https://issues.dlang.org/show_bug.cgi?id=19916

--- Comment #8 from Simen Kjaeraas <simen.kjaras@gmail.com> ---
(In reply to Dennis from comment #7)
> Unless there is a way to actually corrupt memory in @safe code using this (without using @trusted) it's not something @safe should prevent.

Did you see my example code? NotAPointer is perfectly safe to use in @safe code, and presents an interface that encodes that. To reiterate, this may be weird code, but it should be perfectly fine to use in @safe:

struct NotAPointer {
    private static int* p = null;
    private static int len;

    private static void initialize() {
        import core.stdc.stdlib;
        len = 100;
        p = cast(int*)malloc(int.sizeof*len);
    }

    int idx;
    @disable this();
    @trusted this(int i) {
        if (p is null) initialize();
        assert(i >= 0 && i < len);
        idx = i;
    }
    @trusted void callMethod() {
        // We know idx can only be set by the constructor, which checks that
        // it's valid, and initializes p correctly, so no bounds check is
        // necessary at this point.
        p[idx] = 3;
    }
}

There are currently only two ways to make that code do bad things in @safe code, and that's unions and void initialization:


@safe unittest {
    NotAPointer p = void;
    p.callMethod();
}

@safe unittest {
    union U {
        int n;
        NotAPointer np;
    }
    U u = {10};
    u.np.callMethod();
}

--

https://issues.dlang.org/show_bug.cgi?id=19916

--- Comment #9 from Dennis <dkorpel@live.nl> ---
(In reply to Simen Kjaeraas from comment #8)
> Did you see my example code?

Yes, that's why I added "without using @trusted". @safe disallows uninitialized/overlapping pointers, you explicitly circumvented that by casting to an integer type and made it @trusted, and now it breaks. That means your code couldn't be @trusted because you didn't account for overlap, not that overlap of non-pointers is unsafe.

Your second version states:
// We know idx can only be set by the constructor
That's clearly false, that's even the point of the example. A logic bug causing
your @trusted code to break is not a violation of @safe. I'm a bit surprised
that with `@disable this();` void and union initialization is still allowed.
(n.b. you can also do `NotAPointer.init.callMethod();`). Arguably that is an
issue of @disable this.

To me the larger issue is that @trusted is too easily slapped on a function without considering all the ways in which your code can be used. You can't expect every programmer to check that their @trusted code is safe in every imaginable setting / with every possible language feature, when even experienced D programmers get it wrong. (See for example https://github.com/atilaneves/automem/issues/25 or https://issues.dlang.org/show_bug.cgi?id=19777).

In theory, a trusted function should be memory safe any way you call it from safe code. In practice, some trusted functions have oversights or make assumptions about the call site / context, meaning that a function calling @trusted code is also really @trusted instead of safe.

Removing more and more things you can do from @safe code does not seem like a good solution to me. What if I modify idx in your example? You can make it private, but then the entire module is implicitly @trusted. In fact, the whole project becomes @trusted because of __traits(getMember). Shall we make that not @safe too?

--

https://issues.dlang.org/show_bug.cgi?id=19916

--- Comment #10 from Manu <turkeyman@gmail.com> ---
(In reply to Dennis from comment #7)
> (In reply to Manu from comment #6)
> > Accessing uninitialised memory is absolutely a memory safety issue.
> 
> Not per se. This compiles, prints a random number, and doesn't corrupt memory.
> 
> ```
> import std;
> 
> void main() @safe
> {
>     int a = void;
>     writeln(a);
> }
> ```

Hah, well that's obviously broken too!


> > I don't know where this idea that it has strictly to do with pointers comes from? Why would safety be limited that way?
> 
> Paraphrasing Walter from his DConf 2017 keynote, memory safety is not about 'no memory related bugs', it's "a concern in software development that aims to avoid software bugs that cause security vulnerabilities dealing with random-access memory (RAM) access, such as buffer overflows and dangling pointers". Uninitialized / overlapped pointers can cause such issues, uninitialized integers can not.

Accessing uninitialised int's (as above) is possibly the most accessible form ob buffer overflow I can imagine.

> Disallowing a simple harmless sum-type in @safe invites more use of @trusted giving more opportunities for actual memory corrupting bugs to creep in. Not to mention it would break existing code.

It's not harmless at all, interaction with uninitialised memory is *the most
critical form of safety error I can imagine*.
If I were searching for exploits, that's where I would start every time.

> Unless there is a way to actually corrupt memory in @safe code using this (without using @trusted) it's not something @safe should prevent.

int x = void;
array[x]; // boom

--