Fix issue 15645 - Tuple.slice() causes memory corruption.

[tsbockman]

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

If needed, insert a hidden padding member at the start of Tuple slices to ensure they have
the same memory layout as their source.

EDIT: @saurabhdas Has proposed an alternative fix.

I don't recommend it, because it is a breaking change. But, his way does have some other benefits: #3975

[dlang-bot]

Fix	Bugzilla	Description
✓	15645	Tuple.slice() causes memory corruption.

[tsbockman]

Some other issues I noticed with Tuple:

• It has a ton of implementation details that should have been marked private, but weren't. Technically, fixing this would be a breaking change.
• slice() is not callable if this is const. EDIT: My PR now fixes this.
• Shouldn't Tuple actually be implementing opSlice(), anyway?

[MetaLang]

Shouldn't Tuple actually be implementing opSlice(), anyway?

It can't because of the alias this to the embedded built-in tuple which allows compile-time indexing. Adding opSlice would shadow that.

[tsbockman]

I think I finally got the pointer math right - it seems to be working on all platforms now.

[tsbockman]

I posted a poll on dlang's "General" forum to help us decide which fix to use: http://forum.dlang.org/post/inswmiscuqirkhfqlhtd@forum.dlang.org

[schuetzm]

Hmm... instead of inserting a padding at the beginning, wouldn't it be possible to adjust the alignment of all following members to preserve the original layout? The struct equivalents to the tuples are:

struct Original {
    int member1;
    bool member2;
    ubyte[3] __padding1;
    string member3;
}
struct Sliced {
    bool member2;
    ubyte[3] __padding1;
    string member3;
}

The latter is equivalent to:

align(1)
struct SlicedWithAlignment {
    bool member2;
    align(4) string member3;
}

But there's a catch: copying this struct must be disallowed, because that would result in a struct with misaligned members (mostly harmless on x86, but fatal on some other architectures). It would work as long as it's only a reference to the original Tuple, though.

[tsbockman]

@schuetzm I don't really see what problem you're trying to solve with the align suggestion.

What advantage would this have over my current approach, to make up for the obvious limitations?

[schuetzm]

@tsbockman The resulting Tuple would have the expected elements, i.e. the padding wouldn't show up in .tupleof and _traits(allMembers, ...).

[tsbockman]

@schuetzm Using .tupleof or _traits(allMembers, ...) on types you didn't define yourself - without filtering the list down to only the public members - is asking for trouble.

The publicly documented way to get a list of Tuple members is with Tuple.expand and Tuple.Types, neither of which will show the hidden padding member with my PR.

Writing code that depends upon the private guts of someone else's type is always dangerous. Trying to cater to such code in API maintenance essentially makes any change into a breaking change, because nothing is out of reach of such invasive meta-programming tools.

[tsbockman]

I have updated the docs and the unittest example for Tuple.slice to explain that the return type may not be a standard Tuple because of the alignment issues.

[quickfur]

@andralex @JakobOvrum @schveiguy @jmdavis
Pinging random Phobos reviewers here, because this PR touches on some tricky issues and we need more eyes to look at it.

Personally, my feeling is that for slice() to return anything other than Tuple is rather unfortunate, because it breaks type closure. You couldn't pass the result to a function that takes a Tuple, for example, unless it was templated, so the resulting type has rather limited usefulness. Which raises the question of why even bother implementing slice at all. In the ideal case, the result should also be a Tuple, massaged in some way to make it compatible with the original layout, but I'm not sure how to do this, or whether it's even possible. We also don't want to break existing code if we can at all help it, so in that sense this PR has its upsides compared to the other one (which has been closed).

[tsbockman]

I understand why people are not enthusiastic about the approach I have taken in this PR, but in the absence of a better one, we really ought to move forward with this. slice() can always be deprecated later...

[quickfur]

ping @andralex
Need a decision on this, thanks!

[tsbockman]

Ping @andralex .

[tsbockman]

This has been ignored long enough.

[andralex]

I was wondering whether a simpler solution using align would work, see https://dpaste.dzfl.pl/988db9746465

[tsbockman]

I was wondering whether a simpler solution using align would work

Setting all members to align(1) would trivially fix the problem - but introduce performance and compatibility problems, instead.

Using a large fixed alignment is not a solution in general, because there could always be a struct element that requires an even larger alignment.

The implementation of this PR could probably be simplified somewhat if the argument for align could be computed programmatically, as enabled by DMD PR #5750. But, that does not change the fact that Tuple.slice() has to return a non-standard type sometimes, or else be deprecated in favour of a new slicing method that returns by value, instead of by ref.

[andralex]

Setting all members to align(1) would trivially fix the problem - but introduce performance and compatibility problems, instead.

I'm thinking align(4), not align(1). How would that perform on the supported platforms?

[saurabhdas]

Using align(4), Tuple!(ubyte, ubyte, ubyte, ubyte) will have a size of 16 instead of 8. That is not pleasant at all.

I think we should do either:

1. Introduce a new 'slice' which returns by value instead of by reference. Deprecate the current one
2. Add static assert conditions to the current one to fail at compile time instead of causing memory corruption.

[tsbockman]

I'm thinking align(4), not align(1). How would that perform on the supported platforms?

As I already pointed out in the very next sentence after the one you quoted, forcing any fixed alignment will violate the alignment requirements of any element that needs a larger alignment.

In theory the alignment is unbounded. Even in practice, alignments up to 16 are not uncommon.

In the general case, violating the alignment requirements of a type has the potential to crash the program or stymie the garbage collector. This is not a solution.

[tsbockman]

1. Introduce a new 'slice' which returns by value instead of by reference. Deprecate the current one

This is an option, but my fix will still be needed during the deprecation period to prevent memory corruption for people who haven't updated their code to the new by-value slice yet.

2. Add static assert conditions to the current one to fail at compile time instead of causing memory corruption.

This isn't really a solution by itself, IMO. It's allowing implementation details to leak into the public API in a major and unpleasant way.

It could, however, be combined with your solution (1) as a simpler alternative to using my fix during the deprecation period for the by-ref slice.

[andralex]

Here I think we should just return by value. The whole business with suboptimal layout just for the sake of this function seems too much.

[tsbockman]

@andralex @saurabhdas

Here I think we should just return by value.

I'll close this PR then. As this will be a major breaking change, replacement should do all of the following:

1. Add a static assert blocking unsafe usage of the current by-ref method.
2. Deprecate the by-ref method.
3. Add a by-value method with a new name.
4. The PR should link to this discussion and add a note to the module documentation explaining why by-ref can't be implemented in a satisfactory fashion for Tuple, to prevent the whole debate from repeating itself in two years.

I'm a little sceptical that just removing the by-ref functionality will really turn out to be an acceptable solution to the community, but I guess people can complain when they see the deprecation message, if it's a real problem for anyone.