Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow Unsafe Declarations #229

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

skyline131313
Copy link

No description provided.

@mdparker
Copy link
Member

Please add your real name and contact info to the author field.


## Rationale

Foreign code that is interfaced cannot be guaranteed to be safe. It should always be assumed as unsafe. Especially C declarations as the types used are not mangled into the name of the function. It would be possible to link to a C function with the incorrect parameter types.

Choose a reason for hiding this comment

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

Not all extern C/C++ functions are foreign code.

Copy link
Author

Choose a reason for hiding this comment

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

Unless there is a body it is foreign code. If you use C or C++ extern and access the function through a declaration instead of importing it, I don't think that should be a valid use case to consider. You can just import it instead or mark it as a D function and have a different wrapper for C/C++ if you need that.

Choose a reason for hiding this comment

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

That's not true:

// foo.di
extern(C) foo(int i);
// foo.d
extern(C) foo(int i) {
    // look ma, I have a body here
    return i * 2;


## Rationale

Foreign code that is interfaced cannot be guaranteed to be safe. It should always be assumed as unsafe. Especially C declarations as the types used are not mangled into the name of the function. It would be possible to link to a C function with the incorrect parameter types.
Copy link
Contributor

Choose a reason for hiding this comment

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

It should always be assumed as unsafe.

That's the thesis of the DIP, the rationale section should explain in detail why that assumption is useful.

It would be possible to link to a C function with the incorrect parameter types.

You can also link a D function with different signature by giving a false mangle.

Once the OS / build system / foreign code is compromised, there's nothing the programming language can do, so I would say it's best to assume those behave well, because if they don't, all bets are off.

See also: https://forum.dlang.org/post/[email protected]

Copy link
Contributor

Choose a reason for hiding this comment

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

Once the OS / build system / foreign code is compromised, there's nothing the programming language can do, so I would say it's best to assume those behave well, because if they don't, all bets are off.

I’d use the last part to come to the very opposite conclusion. @safe means that the compiler checked it up to @trusted calls. Ideally, this means functions with pragma(mangle,…) should not be allowed to be marked @safe because it simply may not be. If your implementation is @safe, make a shallow wrapper like this:

pragma(mangle, "body")
extern(C) void body_func() @trusted => safe_body();
void safe_body() @safe { … }

It is a little lengthy, but it’s the shortest way which is honest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to think a function definition should always be allowed to be @safe. Only a declaration without a body should be limited to @trusted or @system, if it has non-D ABI or custom mangling. Binary mangling of @trusted and @safe should be merged so they would link to each other even in D mangling.

It still does mean that a false @safe D mangling could be given at the definition site without using @trusted, but that's a small enough hole that i think it's better than requiring the safe_body workaround. If we require the workaround, people are likely to just implement body_func with a @trusted body.

@ntrel
Copy link
Contributor

ntrel commented Aug 1, 2022

Forbidding trusted is both impractical and inconsistent (see the above comments). Forbidding safe is easy to justify, as that means mechanically checked. The DIP rationale might need to be very robust however as it seems from past forum discussions that Walter might oppose that. The DIP would need to address Walters specific criticisms. They can be found in the safe by default DIP forum discussions.

@mdparker
Copy link
Member

I need your name and contact info, please. I don't understand why Walter's name is on it. Could you elaborate?

@anon17
Copy link

anon17 commented Sep 26, 2022

Here's how you can break extern functions:

extern(C) int getpid() @system
{
	//unsafe code here
}

@dukc
Copy link
Contributor

dukc commented Nov 12, 2022

I don't think there is any good reason to prevent annotating an external function @trusted. Consider a safe external function, like abs in the C standard library. Today, it can declared as extern(C) @trusted int abs(int);. With this proposal, this will break, and the declaration will have to be changed to

pragma(mangle, "abs") private extern(C) @system int abs_(int);
@trusted int abs(int arg){return abs_(arg);}

This, in addition to the breakage, is much more verbose and ugly. I don't see what this accomplishes.

Now, preventing @safe on external non-D declarations is a promising idea. Since an external C or C++ function will have to be manually verified for @safety just like a @trusted function, it would make sense that the declaration had to be @trusted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants