Skip to content

issue 15645 - Prevent unsafe usage of Tuple.slice#5342

Merged
dlang-bot merged 1 commit intodlang:masterfrom
tsbockman:issue_15645
May 17, 2017
Merged

issue 15645 - Prevent unsafe usage of Tuple.slice#5342
dlang-bot merged 1 commit intodlang:masterfrom
tsbockman:issue_15645

Conversation

@tsbockman
Copy link
Contributor

@tsbockman tsbockman commented Apr 21, 2017

This is a mitigation for Phobos issue 15645 - Tuple.slice() causes memory corruption.

Through several previous discussions, the community reached the conclusion that there is no good solution to this problem - that is, one which preserves all of the following desirable characteristics:

  1. Breaks no currently working code.
  2. Actually fixes those cases which currently silently corrupt data.
  3. Optimal (or very close) performance in all cases.
  4. Tuple slices are just 100% normal Tuples.
  5. Simple and implementable in very few lines of code.

The candidates solutions were:

A) Front-pad Tuple by-ref slices as needed to correct the alignment of the members. (Violates 4 & 5)
B) Slice by-value instead. (Violates 1 & 3)
C) Prevent unsafe usage with a static assert at compile time. (Violates 2)

I strongly favour (A), but my work was vetoed by @andralex , theoretically in favour of (B). However, no one has stepped forward to actually implement (B) and in the mean time Tuple is still silently corrupting data.

So, I offer (C) as a temporary stop-gap, either until someone implements (B), or the community decides to revisit my original proposal.

This PR is small, simple, and a strict improvement over the current situation. Replacing it with (A) or (B) at a later date should not break any code that wouldn't have been broken anyway.

@dlang-bot
Copy link
Contributor

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

@quickfur
Copy link
Member

LGTM. Deferring to @andralex for merging, though.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

This is a very nice idea. Do you think you could replace the use of inout with the template this parameter? https://dlang.org/spec/template.html#TemplateThisParameter

I continue to get feedback from users that find inout inscrutably complex and difficult to understand and use. Less use of it in the standard library would be great. Thanks!

@tsbockman
Copy link
Contributor Author

Do you think you could replace the use of inout with the template this parameter?

Because the return type is different from the type of this, I don't think that will lead to clearer code. Wouldn't I then need a template that emulates the functionality of inout to copy any const-ness qualifiers over?

I know that inout causes a lot of confusion, but sometimes it's just the right tool for the job.

This is a very nice idea.

I figure if we actually ship this with the next release, then either some Tuple users will come out of the wood-works to clamour for one of the two long-term solutions - or we'll prove that no one cares.

@andralex
Copy link
Member

@tsbockman I'm thinking of returning ref auto, would that work?

@tsbockman
Copy link
Contributor Author

I'm thinking of returning ref auto

Because the return type is being applied by a manual, @system cast, the function would still need to manually compute the correct const-ness. It could be done, but it would be a lot more code (and possibly template bloat).

@andralex
Copy link
Member

andralex commented May 17, 2017 via email

@tsbockman
Copy link
Contributor Author

@andralex So, is this ready to merge, then?

@andralex
Copy link
Member

andralex commented May 17, 2017 via email


// Phobos issue #15645
Tuple!(int, short, bool, double) b;
static assert(!__traits(compiles, b.slice!(2, 4)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much about this code, but isn't the 4 out of bounds? Or are the arguments 1-indexed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the docs:

to = A size_t designating the ending position (exclusive) of the slice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks

@tsbockman
Copy link
Contributor Author

Thanks, guys.

@tsbockman tsbockman deleted the issue_15645 branch May 17, 2017 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants