Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

fix unsafe .ptr accesses in druntime#1590

Merged
yebblies merged 1 commit intodlang:masterfrom
WalterBright:fixUnsafePtr
Jun 13, 2016
Merged

fix unsafe .ptr accesses in druntime#1590
yebblies merged 1 commit intodlang:masterfrom
WalterBright:fixUnsafePtr

Conversation

@WalterBright
Copy link
Member

array.ptr is no longer considered @safe

Needed for dlang/dmd#5860

@WalterBright WalterBright changed the title fix unsafe .ptr access in core.internal.string fix unsafe .ptr accesses in druntime Jun 12, 2016

private const(void)* arrayToPtr(const void[] array) @trusted
{
// Ok because the user will never dereference the pointer
Copy link
Contributor

Choose a reason for hiding this comment

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

Not ok... If safe code can call this method and thereby break memory safety, it should not be trusted. The fact that the callers above don't do this is just a convention - they are effectively @trusted thanks to this function.

Copy link
Member

Choose a reason for hiding this comment

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

What about this instead:

private size_t arrayToPtr(const void[] array) @trusted { return cast(size_t)array.ptr; }

Copy link
Member

Choose a reason for hiding this comment

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

Hm... probably should return ptrdiff_t instead of size_t, since pointer arithmetic is signed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, anything that prevents it from being dereferenced in @safe code.

Copy link
Member

Choose a reason for hiding this comment

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

probably should return ptrdiff_t

I take it back, needs to be size_t because of the code in _enforceNoOverlap

@WalterBright
Copy link
Member Author

@yebblies we had a spirited debate about this a couple years ago, and I started out in your position. A consensus eventually emerged that @trusted code should be shrink-wrapped around the unsafe operation, so that safety inference can be properly done with the rest of the code, even though the rest of the code still has to behave itself. You'll see this in other places with paired malloc/free in separate @trusted blocks.

I made the function private to keep it local.

@schveiguy I've used machines where size_t is not the same size as void*, even in 32 bit mode. The trouble with assuming they are the same size is some very weird bugs can silently appear if such a machine is ported to. I have developed an aversion to such equivalences after having my kingdom blasted by them.

@yebblies
Copy link
Contributor

I think there were two parts to that discussion - one was using @trusted lambdas, to allow inference to work on the rest on the function. The other was @trusted helper functions that did not have a @safe interface. The former is a necessary evil, the latter is not. Either way they effectively make the entire calling function @trusted, at least the former will include the text @trusted somewhere inside the body.

The whole idea that @trusted can be grepped for, and the function manually audited, breaks down when the function doesn't provide a @safe interface. That's what the auditing is meant to prevent, and that's what I'm objecting to now.

@WalterBright
Copy link
Member Author

I just realized instead of size_t we can use uintptr_t, and no portability or trusted issues.

@yebblies
Copy link
Contributor

Auto-merge toggled on

@yebblies yebblies merged commit e13b051 into dlang:master Jun 13, 2016
@WalterBright WalterBright deleted the fixUnsafePtr branch June 13, 2016 09:24
@schveiguy
Copy link
Member

Thanks, I had not heard of uintptr_t. Definitely the perfect answer.

Side question: will D ever support such systems? I have worked with 8-bit CPUs where the pointer type is 16-bits (D could potentially support this, but never druntime), but never seen something of modern 32-bit or higher that does this.

@schveiguy
Copy link
Member

Hm.. arrayToPtr is actually quite a nice feature for @safe, especially if dlang/dmd#5860 goes through (and even for using in @system code where you want the added precaution). What about adding this to object, and name it something short, like sptr? Using UFCS, just call arr.sptr to use it.

@ntrel
Copy link
Contributor

ntrel commented Jun 13, 2016

What about adding this to object, and name it something short, like sptr?

I think we need something like this at least for porting existing code. Although maybe named a bit clearer e.g. ptrValue. std.array might be a good place. As mentioned in the bug report, this is useful when the user doesn't want to dereference the pointer, just compare it against another slice's .ptr.

edit: Or if needed in druntime, maybe a new module, core.compat. std.typecons.octal could be moved there, maybe other things would fit too.

@denis-sh
Copy link
Contributor

Could someone please explain in T[] to const T[] conversion reasons?

@yebblies
Copy link
Contributor

Could someone please explain in T[] to const T[] conversion reasons?

Walter's preferences... And possibly some vague future plans to change what in means...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants