Bug 11176 – array.ptr in @safe code may point past end of array

The following program is @safe, yet reads from an invalid memory location: --- @safe: ubyte oops(ubyte[] b) { return *b.ptr; } void main() { oops(new ubyte[0]); // - or - auto b = new ubyte[42]; oops(b[$ .. $]); } --- To keep memory safety guarantees, we would at least have to make sure that the element after the end of an array never belongs to a different, valid allocation. Brought up by Denis Shelomovskij in a discussion on GitHub.

Forgot to mention: The above snippet compiles fine using DMD 2.064 Git (a35bd9e) on Linux x86_64.

The easiest fix would be to just disallow accessing the .ptr property in @safe code. There probably wouldn't be much reason to use it in the first place anyway.

An interesting side note to marking .ptr on arrays as unsafe would be that it would make it kind of pointless to mark a lot of C functions as @trusted like some folks have been trying to do in druntime (and incorrectly in some cases - e.g. bug# 11168), since then the function making the call would likely have to be marked as @trusted or @system quite often when calling C functions, as many C functions take arrays. And many that don't take arrays still take pointers of another kind, and taking the address of something is @system, so it makes me wonder if we should just not mark any C functions as anything but @system. It's already arguably bad to mark them as @trusted when we don't have their source code, and when you pretty much can't call them from @safe code anyway, it becomes rather pointless to try and mark them as @trusted.

@J(In reply to comment #3) > An interesting side note to marking .ptr on arrays as unsafe would be that it > would make it kind of pointless to mark a lot of C functions as @trusted I think you are missing something here: A C function taking an array by pointer and length or a pointer to a zero-terminated array can never be @trusted.

> I think you are missing something here: A C function taking an array by > pointer and length or a pointer to a zero-terminated array can never be > @trusted. Ah, that would be true. So, regardless of ptr, it's a problem. Though with druntime using @trusted: on whole modules in some places, I find it hard to believe that we're not screwing this up in several places. And with so many C functions taking pointers of one variety of another (combined with the fact that we don't have the actual source code for C functions normally), I'd be tempted to argue that marking C functions with @trusted should simply not be done under normal circumstances.

(In reply to comment #5) > And with so many C > functions taking pointers of one variety of another (combined with the fact > that we don't have the actual source code for C functions normally), I'd be > tempted to argue that marking C functions with @trusted should simply not be > done under normal circumstances. I invite you to take up this argument with the people who are working hard to make Phobos usable in @safe code. ;) Jokes aside, C functions that are @safe should be marked as such, otherwise this will just lead to client code being marked as @trusted. And this not only shifts the problem, but as the expected fan-in of a druntime declaration is larger than one, makes the problem *worse* because now that difficult decision has to be made in several places instead of just one. I agree that marking functions as trusted (external or not) comes with a high potential for mistakes, but I'd argue that the best way to avoid making them is to help with reviewing the related pull requests (and the existing code once new pitfalls such as the reentrancy issue are discovered).

(In reply to comment #6) True enough, but I think that it highlights how careful we need to be with marking functions @trusted. Sometimes, I think that we're too quick to try and mark things as @trusted just so that something will work in @safe code. Certainly, it's something that we need to be very careful about in reviews - especially when the compiler clearly considers a number of things @safe which aren't.

(In reply to David Nadlinger from comment #2) > The easiest fix would be to just disallow accessing the .ptr property in > @safe code. There probably wouldn't be much reason to use it in the first > place anyway. Maybe, but it should be safe to allow comparing a .ptr with another pointer, so long as .ptr is not dereferenced. So comparisons are fine, dereference should be disallowed, and copying/escaping .ptr disallowed. I expect this may be worth implementing to limit code breakage better than disallowing .ptr entirely.

One another way I can think is, array.ptr property would add a hidden check `arr.length != 0` under @safe code, then returns `null` instead when the length is 0. @safe ubyte* oops1(ubyte[] b) { return b.ptr; } @safe ubyte oops2(ubyte[] b) { return *b.ptr; } void main() { auto b = new ubyte[42]; assert(oops1(b[0 .. $]) is &b[0]); assert(oops1(b[0 .. 1]) is &b[0]); assert(oops1(b[0 .. 0]) is null); // the 'safer' behavior // With the proposed behavior, this call will cause null pointer dereference, // then it's deterministic and does not cause undefined behavior. oops2(b[0 .. 0]); }

(In reply to Kenji Hara from comment #9) > One another way I can think is, array.ptr property would add a hidden check > `arr.length != 0` under @safe code, then returns `null` instead when the > length is 0. As someone who lives in the camp of "empty arrays and null arrays should be treated the same", I think this behavior wouldn't affect me. However, many significant people depend on this NOT being the case. Think of the pushback for the if(!arr) fix. To make the behavior different if you add a @safe tag may not affect them, but since the compiler can *infer* safety, this will be bad for anyone. Imagine you have template code not marked @safe, and you find a legitimate use for arr[$ .. $].ptr. The compiler may infer @safe, and then your code fails at runtime even though it would pass if not inferred @safe. In order to avoid such an issue, I think you have to just disallow ptr access in @safe code. That's the only thing that's checkable at compile-time, and will prevent a @safe inference.

It was just an idea. Indeed, it would be a case that the conditional behavior is worse than simple disabling. I noticed that we already have equivalent but safer way &arr[0]. Under @safe attribute, it checks the 'arr' boundaries and throws RangeError if the index access is invalid. So, disabling arr.ptr would not have so big impact.

(In reply to Kenji Hara from comment #11) > I noticed that we already have equivalent but safer way &arr[0]. Under @safe > attribute, it checks the 'arr' boundaries and throws RangeError if the index > access is invalid. > > So, disabling arr.ptr would not have so big impact. Excellent point! I think this solidifies the position we should have.

(In reply to Steven Schveighoffer from comment #10) > I think you have to just disallow ptr > access in @safe code. That's the only thing that's checkable at > compile-time, and will prevent a @safe inference The following is safe: assert(elem.ptr is null); Even this is safe: i = tmp.ptr - trailing.ptr; Both of these are from Phobos. We only need to prevent dereference of .ptr, and aggressively so. But reading the pointer itself is OK so long as the address doesn't escape to another pointer.

(In reply to Nick Treleaven from comment #13) > The following is safe: > assert(elem.ptr is null); > > Even this is safe: > i = tmp.ptr - trailing.ptr; > > Both of these are from Phobos. We only need to prevent dereference of .ptr, > and aggressively so. But reading the pointer itself is OK so long as the > address doesn't escape to another pointer. I agree these could be possible rules that would be safe. However, this would be confusing, since pointer dereferencing is allowed in safe code. Is there a more reasonable way to explain this? I think it's easier to explain, and more consistent to just prevent access to ptr. Especially when there are workable alternatives.

In general, we should not make something @system unless it needs to be, but it's not like we guaranteed that the compiler wasn't conservative in what it considered to be @safe. So, losing out on some potentially @safe operations with ptr in order to make it more straightforward for the compiler to detect @system uses of it and to make it easier for the programmer to understand it is something that we can do if we deem it appropriate. Simply marking .ptr as @system at all times would be kind of like marking & on local variables as @system. Sure, what you're doing with the pointer could be @safe, but it can't necessarily guarantee that what you're doing with it is @safe. And the compiler is not going to treat stuff like int foo; auto result = &foo is null; or int foo; int bar; auto result = &bar - &foo; as @safe. You're taking the address of a local variable, and it considers that @system. Just because you're doing something @safe with the resulting pointer does not mean that it's going to consider it @safe. That's up to you to figure out. So, I'm inclined to argue that .ptr should just always be considered @system like taking the address of a local variable is always considered @sytem. It's simple and staightforward, and if the code in question is obviously @safe to the programmer, then it'll be easy for them to figure out that it's okay to mark it @trusted. And we seriously reduce the risk of screwing up and allowing operations that aren't actually @safe into @safe code.

> it should be safe to allow comparing a .ptr with another pointer, so long as .ptr is not dereferenced I think I've pushed this point enough. Instead I expect it is acceptable to add a function to std.array: void* pointer(T)(T[] arr) @trusted; assert(arr.pointer is null); // @safe Then although fixing existing safe code may be tedious, at least it's straightforward.

There is another way: b.ptr // unsafe &b[0] // safe - because array bounds checking will verify you have a good pointer

I added a possible mechanism to allow safe equivalent to arr.ptr. Please see PR: https://github.com/dlang/druntime/pull/1592

*** Issue 16266 has been marked as a duplicate of this issue. ***

Commit pushed to master at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/02d7ebe3f37b3584cfd0443630d3493f49411c0e Merge pull request #5860 from WalterBright/fix11176 fix Issue 11176 - array.ptr in @safe code may point past end of array

Commit pushed to stable at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/02d7ebe3f37b3584cfd0443630d3493f49411c0e Merge pull request #5860 from WalterBright/fix11176

I think this is already fixed, not sure why it didn't get closed.

> I think this is already fixed, not sure why it didn't get closed Because it seems like the bugzilla integration doesn't like whitespace: "fix Issue 11176 - array.ptr in @safe code may point past end of array"