fix Issue 18554 - tupleof ignoring private shouldn't be accepted in @…

[WalterBright]

…safe code

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18554	normal	tupleof ignoring private shouldn't be accepted in @safe code

[wilzbach]

Nice! Yet another hole fixed :)

[wilzbach]

Nit: Shouldn't this be named onlyunsafeaccess, onlysystemaccess or nosafeaccess as it prevents @safe access but allows unsafe access?

[JinShil]

Yeah, that's going to bite me later, I fear.

[wilzbach]

#8041

[rainers]

Is this a good idea? It broadens the meaning of @safe beyond memory safety. This might open a can of worms for other code considered unsafe.
I would also expect an update to the documentation and tests that show that usual access is still ok.

[JinShil]

@WalterBright Please consider leaving PRs open for at least 24 hours to allow time for everyone to have their say.

[timotheecour]

one example use case of tupleof is for serialization/deserialization routines that must rely on private member access; there's nothing inherently unsafe about these (in terms of memory safety).
this PR would prevent such use cases in safe code

[aG0aep6G]

I don't think this should be tied to DIP 1000 (global.params.vsafe). DIP 1000 is about scope. But this has nothing to do with scope.

[wilzbach]

In other precendents breaking changes were introduced with -dip1000 as it's effectively unused, e.g. #8036 (there were more in the last weeks, but I can't find them on the first try).
I guess eventually dip1000 might evolve in a sublanguage :/

[jacob-carlborg]

Yet another regression without thinking of the consequences 😞. Come on people, can we please do better than this!

[wilzbach]

Yet another regression without thinking of the consequences disappointed.

It's not a regression (per se), because it's only active in the transitional -dip1000 flag which no one uses at the moment. For -dip1000 breaking changes are expected and can be done.

Regarding the motivation - check Bugzilla:

Copy/Paste

---

But tupleof ignores private even in @safe code. So it can be used to violate the assumption, which leads to memory corruption.

The File example in code (https://run.dlang.io/is/1QSsUk):

void main() @safe
{
    import std.stdio: File, writeln;
    auto hosts = File("/etc/hosts");
    {
        auto hosts_copy = hosts;
        hosts_copy.tupleof[0].refs = 1; /* uh-oh */
    }
    auto self = File(__FILE__);
    writeln(hosts.rawRead(new char[1000]));
        /* Reads from __FILE__ instead of /etc/hosts. */
}

And the issue reduced to its core (https://run.dlang.io/is/reMMQt):

--- foo.d
struct S
{
    private int* p;
    this(int x, int y) @safe { p = &[x, y][0]; }
    int get2nd() @trusted { return p is null ? 0 : p[1]; }
        /* Assuming that p is only ever set by the constructor.
        So it's either null or there must be two elements. */
}

--- bar.d
import foo;
import std.stdio;
void main() @safe
{
    auto s = S(1, 2);
    s.tupleof[0] = &[10][0]; /* Should not be allowed. */
    writeln(s.get2nd()); /* garbage */
}

[jacob-carlborg]

It's not a regression (per se), because it's only active in the transitional -dip1000 flag which no one uses at the moment.

To be honest, I missed it was only for DIP1000.

For -dip1000 breaking changes are expected and can be done.

How is this going to be handled when dip1000 is the default? Doesn’t sound likely that every change that dip1000 introduced can be turned into a deprecation.

How are the examples related to memory safety?

BTW, I don’t think that anyone would accidentally use tupleof. That other things dip1000 protests against is more likely to happen by accident.

[WalterBright]

@safe code should not be accessing private members in other modules, and using tupleof to "work around" that restriction is a giant hole in the @safe system. Such code should be marked @trusted or @System.

[WalterBright]

BTW, I don’t think that anyone would accidentally use tupleof. That other things dip1000 protests against is more likely to happen by accident.

The idea is @safe offers a guarantee, not a "guarantee unless you use tupleof". The latter introduces a hole that will be very hard for reviewers to find, and impossible to guard against.

[WalterBright]

It broadens the meaning of @safe beyond memory safety.

A struct may present an @safe interface to users, but internally have private unsafe pointers. tupleof allows any code to have access to those private members, and can mess up the invariants relied on by the struct. Even int fields are not safe to access, as they may be used as an index to a pointer.

If you're going to access private members in another module, bypassing the safe interface provided by that module, you're in @System land.

[WalterBright]

How is this going to be handled when dip1000 is the default?

Presumably by adding the equivalent of a -nodip1000 switch.

[rainers]

Accessing private data is not memory unsafe by itself, though it might corrupt data in other ways. @safe has never been about encapsulation so far, though.

Please note that this no longer compiles with -dip1000:

struct S
{
	private int x;
}

void main() @safe
{
	import std.stdio;
	S s = S(7);
	writeln(s);
}

Error:

safe.d(11): Error: @safe function D main cannot call @system function std.stdio.writeln!(S).writeln

[wilzbach]

That error is about phobos still not working with -dip1000.

It's the same for 2.079: https://run.dlang.io/is/mAwL0j

[rainers]

Ah, ok. I just tried it against 2.078 which still accepts it.

BTW: "latest" in the list of all compilers on run.dlang.io seems to be 2.078, while "nightly" seems to be about a month old.

[rainers]

If you're going to access private members in another module, bypassing the safe interface provided by that module, you're in @System land.

That is an encapsulation issue that @safe has not been about so far. Maybe it is ok to add it, but it is not a small addition that is part of escape analysis. It sure needs an update to the documentation.

[wilzbach]

BTW: "latest" in the list of all compilers on run.dlang.io seems to be 2.078, while "nightly" seems to be about a month old.

Hmm, it gets updated daily by a cron which still seems to work:

https://run.dlang.io/is/zx3psE
https://run.dlang.io/is/O2MtyK

How did you infer that it's outdated?

[WalterBright]

Accessing private data is not memory unsafe by itself

Yes it is. Please refer to my previous explanation as to why. Encapsulating unsafe code and providing a safe interface is essential, and @safe code must respect the encapsulation.

[rainers]

How did you infer that it's outdated?

If I add --version as a command line option to dmd-nightly, I get DMD64 D Compiler v2.079.0-beta.1-master-75b460e which seems to be 21 days old according to the sha: https://github.com/dlang/dmd/search?q=hash%3A75b460e&type=Commits&utf8=%E2%9C%93.

[rainers]

Encapsulating unsafe code and providing a safe interface is essential, and @safe code must respect the encapsulation.

That might be true but there is nothing about encapsulation in the documentation of @safe: https://dlang.org/spec/function.html#function-safety.
You can still violate safety if the function sits in the same module as the private member. So far these concepts have been orthogonal.

[WalterBright]

You can still violate safety if the function sits in the same module as the private member.

That's why I said "access private members in another module".

Code in the same module has always been allowed to access private members, this is quite deliberate. The idea is that it eliminates the need for the awful C++ "friend" hack, and presumably the programmer of the module knows what he's doing. Importing a module is another thing entirely.

So far these concepts have been orthogonal.

Encapsulation of system code is fundamental to D's safety system.

[rainers]

Encapsulation of system code is fundamental to D's safety system.

I don't doubt that. But just to repeat mself: the language specification does not mention that @safe can change encapsulation.

So far the story has been: "If you want to scrutinize the safety of your code marked @safe, just grep for @trusted functions". As the example shows it is actually "grep for @trusted functions and if it uses non-local state check the whole module". Granted, without the change in this PR, you'd need to check the whole code base.

[aG0aep6G]

As the example shows it is actually "grep for @trusted functions and if it uses non-local state check the whole module".

Maybe this warrants a new visibility attribute that's more restrictive than private, not allowing access from the module. You'd still have to check the whole aggregate, of course.

Or let @system apply to variables, meaning that all accesses become @system. Then you couldn't even mess up in @safe methods of the same aggregate.

[John-Colvin]

@WalterBright sounds like in this case the module is the unit of @safe code, not the function, and that therefore the guarantees on any individual @safe function depends on all code (even @system) code being unable to mess up state. Sounds more like care & convention and less like mechanical checking.

(this would be true at aggregate scope even if accessing private was limited to the same aggregate, so that's not the problem)

[John-Colvin]

This leads to a wider question about what "valid state" is when passed to an @safe function (the this reference being the state in question in this PR)