Fix 23175 - -preview=in silently adds possible stack memory escape

[dkorpel]

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	23175	enhancement	-preview=in silently adds possible stack memory escape

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14219"

[maxhaton]

@Geod24

[thewilsonator]

Should this target stable?

[kinke]

This would break a perfectly valid:

@nogc @safe:
void foo(in string s) {}
void main() { foo(['a']); }

[schveiguy]

As I commented in the bug report, the current situation is making a breaking change on existing valid code without a warning to give the user a heads-up that this might happen. It's especially egregious because it's introducing memory corruption, which we all know is the worst form of bug to silently introduce.

So the issue is valid.

Now, regarding this PR, I'm not sure I agree with it. I prefer a warning to the user that this may happen in the case of a parameter that is marked in, but not return, yet is returned. I don't know how to make this happen, or if it's even feasible. But the feature itself, along with the optimization, should be allowed. Just warn when this is happening (maybe you can silence the warning with another switch?)

[Geod24]

I prefer a warning to the user that this may happen in the case of a parameter that is marked in, but not return, yet is returned.

But why would we do this for in, but not for scope ?

[dkorpel]

I've now changed it into a transition flag.

[schveiguy]

But why would we do this for in, but not for scope ?

Because scope is defined as "may not escape". in is defined as const. As of today, an in parameter may escape, and the compiler does not optimize away allocations based on that. With scope, it was feasible the compiler might do this, even if it didn't, so it's on you for relying on implementation details. With in, it wasn't legal for it do do this, until you changed the definition of in.

In other words, the feature turns well-defined code into undefined behavior, without any warning.

[Geod24]

in is defined as const.

Except for a brief period (< 5 months) in 2018, in has always be defined as const scope, as @pbackus pointed out.

With in, it wasn't legal for it do do this, until you changed the definition of in.

Actually, it's the other way around: I changed the definition of in to mean "const when -preview=in is not used".

[schveiguy]

As I pointed out it has been const since feb 2021.

On Jun 16, 2022, at 12:26 PM, Mathias LANG @.***> wrote:

 in is defined as const.

Except for a brief period (< 5 months) in 2018, in has always be defined as const scope, as @pbackus pointed out.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

[Geod24]

@schveiguy : No, you said that before I changed the meaning of in, it wasn't legal for it do do this (this being for the compiler to consider the parameter may not escape). That's plain wrong.

@dkorpel : I don't think that PR adds value, because the basis for the issue is incorrect. The issue triggers with scope as well. The change you've made to detect those trivial cases is IMO good, although I'm not sure everyone would agree ;) If I'm not mistaken, said change will catch this case, correct ? Then we have -transition=in to list all instances of in, which IMO is better as the ref is likely to be more "surprising" to the user than scope.

[schveiguy]

In means const. Is it legal to make this optimization with const instead of in? If not then it’s not legal for in. “ When not in use, in is equivalent to const.”

On Jun 16, 2022, at 6:47 PM, Mathias LANG @.***> wrote:

 @schveiguy : No, you said that before I changed the meaning of in, it wasn't legal for it do do this (this being for the compiler to consider the parameter may not escape). That's plain wrong.

@dkorpel : I don't think that PR adds value, because the basis for the issue is incorrect. The issue triggers with scope as well. The change you've made to detect those trivial cases is IMO good, although I'm not sure everyone would agree ;) If I'm not mistaken, said change will catch this case, correct ? Then we have -transition=in to list all instances of in, which IMO is better as the ref is likely to be more "surprising" to the user than scope.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[dkorpel]

because the basis for the issue is incorrect. The issue triggers with scope as well.

I don't have skin in this game, so I wouldn't mind if the issue was closed as WONTFIX. However, I do think @schveiguy has a point: it's reasonable to think legacy code has been written that assumes in parameters may escape the function. It's less reasonable to think this of scope, since the attribute's sole meaning is 'does not escape'. Yes, a preview switch means breaking changes are allowed, but usually that entails introducing new errors like -preview=nosharedaccess, or new edge case behavior like -preview=intpromote. Possibly introducing memory corruption without warning is a bit more severe than that, so a transition helper is in its place.

The change you've made to detect those trivial cases is IMO good, although I'm not sure everyone would agree ;)

The change: https://github.com/dlang/dmd/pull/14221 I haven't heard any disagreement about this idea on the news group, what do you think the objection is?

If I'm not mistaken, said change will catch this case, correct ?

When 'this case' is string foo(in string s) {return s;} then yes. I had to change the test case in the test suite.

Then we have -transition=in to list all instances of in, which IMO is better as the ref is likely to be more "surprising" to the user than scope.

I thought adding ref was supposed to silently work without problems.

[Geod24]

@schveiguy

In means const. Is it legal to make this optimization with const instead of in? If not then it’s not legal for in. “ When not in use, in is equivalent to const.”

So does the problem manifest when -preview=in is not in use ?

@dkorpel :

it's reasonable to think legacy code has been written that assumes in parameters may escape the function

That's where we disagree I think. in has been defined as const scope since 2013. I don't think it was ever reasonable to assume it could escape.

I haven't heard any disagreement about this idea on the news group, what do you think the objection is?

Walter has been consistently opposed to the idea of bringing scope diagnostic to @system functions.

I thought adding ref was supposed to silently work without problems.

I'm not sure if you are being ironic or not... :) What I wanted, and the way it currently works, is that in (with -preview=in) means const scope ref, but sometimes decay to const scope if the compiler thinks that it's more efficient to pass by value (e.g. int, bool, etc...). While some people agreed with this approach (@kinke for example), some people were vocally opposed to it (Andrei). So the consensus we reached in one of the DLF meetings was to disable the optimization, so it would always mean ref. I'm going to implement it soon (as well as start preparation to enable -preview=in by default) as soon as #12242 is merged.

[Geod24]

@dkorpel :

Possibly introducing memory corruption without warning is a bit more severe than that, so a transition helper is in its place.

The change that introduced the memory corruption is the one that allowed a scope array parameter to an array function to be allocated on the stack. There's a million way you can demonstrate "memory corruption" if you enable -preview=in without -preview=dip1000, because there's a million way you can demonstrate memory corruption without -preview=dip1000. I'm all for making the transition as painless as possible, but this is not specific to -preview=in.

[dkorpel]

Walter has been consistently opposed to the idea of bringing scope diagnostic to @system functions.

One of his latest comments suggested otherwise: https://forum.dlang.org/post/[email protected]

I'm not sure if you are being ironic or not... :)

I'm not, I'm trying to recall what the problem with adding ref was. I think it was mutable aliasing:

void f(in string x, ref string y) 
{
    const copy = x;
    y = "change";
    assert(x == copy); // when calling `f(s, s)`, succeeds or fails depending on whether `in` means `ref`
}

[Geod24]

One of his latest comments suggested otherwise: https://forum.dlang.org/post/[email protected]

Haven't seen that one, great :)

I'm not, I'm trying to recall what the problem with adding ref was. I think it was mutable aliasing: [...]

Yes it was. A problem that exists currently. The main point being that one couldn't "rely" on in being ref, so we might see "different behavior". I pointed out that this problem also exists for auto ref, that @live is supposed to fix this (how I don't know, but that was the promise ;) ) and that this is just bad code hat would break anyway (logically break, but not corrupt memory).

Nevertheless, I'd rather loose the optimization that the whole of -preview=in, so I'll disable it for DMD.

[schveiguy]

@schveiguy

In means const. Is it legal to make this optimization with const instead of in? If not then it’s not legal for in. “ When not in use, in is equivalent to const.”

So does the problem manifest when -preview=in is not in use ?

No because without the preview switches, the behavior is defined. In is currently const, as the quote says. With preview in it becomes scope const. If there are cases such as this one where preview in turns pre existing valid code into undefined behavior, we need to warn on that behavior. It doesn’t mean we have to disable the optimization, we just need to make sure the user is aware of what will happen if they don’t remedy the situation.

it's reasonable to think legacy code has been written that assumes in parameters may escape the function

That's where we disagree I think. in has been defined as const scope since 2013. I don't think it was ever reasonable to assume it could escape.

This is not true. It’s also not an opinion, it’s a fact. In is defined as const since at least February 2021 in the spec, much longer in the implementation, IIUC.

[dkorpel]

Closing this because this is stalled. I think it's better to pursue Paul Backus' suggestion:

The most straightforward fix would be to have the compiler emit a warning any time an in parameter escapes from a function in @system or @trusted code (i.e., the same thing it currently emits an error for in @safe code).

https://issues.dlang.org/show_bug.cgi?id=23175#c12