Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zero sized string having address null have unexpected results when slicing and taking the pointer #1831

Closed
Hejsil opened this issue Dec 14, 2018 · 17 comments
Milestone

Comments

@Hejsil
Copy link
Contributor

Hejsil commented Dec 14, 2018

test "" {
    var data: ?[*]const u8 = ""[0..].ptr;
    @import("std").debug.assert(data != null);
}
zig/std/debug/index.zig:119:13: 0x205029 in ??? (test)
            @panic("assertion failure");
            ^
test.zig:3:32: 0x205060 in ??? (test)
    @import("std").debug.assert(data != null);
                               ^
std/special/test_runner.zig:13:25: 0x22354a in ??? (test)
        if (test_fn.func()) |_| {
                        ^
zig/std/special/bootstrap.zig:112:22: 0x223203 in ??? (test)
            root.main() catch |err| {
                     ^
zig/std/special/bootstrap.zig:49:5: 0x222fc0 in ??? (test)
    @noInlineCall(posixCallMainAndExit);
@thejoshwolfe
Copy link
Contributor

The ptr field of a 0-len slice has an undefined value. The reasoning is that you're not allowed to read or write at the pointed-to location (since there are 0 items there, any access would be out of bounds.), so the value of the pointer is not meaningful.

Another way to think about this is "where in ram can you find 0 consecutive u8s?", to which the answer is "everywhere". Starting at literally any address you have at least 0 consecutive objects of every type. (This might not be the most helpful explanation.)

The main issue here is that Zig pointers are not necessarily memory addresses at all. The semantics of a pointer are that a pointer tells you where you can find the thing you're pointing at. If you're pointing at nothing, then the pointer doesn't need to do anything.

This is also the reasoning behind @sizeOf(*u0) == 0. The type system guarantees that this pointer is useless, so the storage for the pointer value is elided.

@andrewrk
Copy link
Member

To further clarify Josh's point, this code is equivalent to:

test "" {
    var data: ?[*]const u8 = undefined;
    @import("std").debug.assert(data != null);
}

@Hejsil
Copy link
Contributor Author

Hejsil commented Dec 15, 2018

Actually no, my example should be equal to this:

test "" {
    var a: [*]const u8 = undefined;
    var a_opt: ?[*]const u8 = a;
    @import("std").debug.assert(a_opt != null);
}

Which it isn't for some reason (this code works). This code doesn't work either:

test "" {
    var b: []const u8 = ([*]const u8)(undefined)[0..0];
    var b_opt: ?[*]const u8 = b.ptr;
    @import("std").debug.assert(b_opt != null);
}

Which doesn't really make sense. The values of the slice should be {.ptr = undefined, .len = 0 }, same as the ptr example, but this one fails.

@Hejsil
Copy link
Contributor Author

Hejsil commented Dec 15, 2018

Anyways, I think this is a problem with the fact that 0 is null for ptrs, so if I have a ptr that is already null (even though it is not nullable) then assigning it to an optional will mess up the semantics.

@thejoshwolfe
Copy link
Contributor

All of your code examples branch of an undefined value, which is undefined behavior in Zig and C. The equivalent C code is:

void main() {
    char *p;
    assert(p != NULL);
}

The fact that optional types have special semantics for pointers is for C compatibility. If a C function takes or returns a pointer that might be NULL, then Zig needs a way to express that. If optional pointers in Zig used the normal optional semantics where it becomes a struct of {non_null: bool, payload: T}, then that structure wouldn't be suitable for interfacing with C, but it would pass all the tests involving coercing an undefined pointer to an optional pointer.

#1059 will introduce a special pointer type for C compatibility, so maybe this special case handling for optional pointers could be isolated to that type and avoid this confusion, but that remains to be seen.

Another thing that might be adding confusion to all this is that I see in the original post you're taking the address of an empty string. In C an empty string is not really empty; it includes the '\0' byte at the end. But in Zig "" or any other empty array is actually empty, so a pointer to it is unreadable, whereas in C you would be allowed to read a pointer to an empty string and find the null byte.

@Hejsil how did you run into this issue?

@Hejsil
Copy link
Contributor Author

Hejsil commented Dec 15, 2018

@thejoshwolfe I have a struct like this:

struct {
   ptr: ?[*]const u8,
   len: u28,
}

and depending on the API used (streaming vs non streaming) ptr would either point to some data (non streaming) or be null and it is up to the user to keep track of the data (streaming). I was writing a test wherelen is 0, so I just needed ptr to be not null, so I did this:

T{ .ptr = ""[0..].ptr, .len = 0 }

I understand that our pointer optimization is what is causing this weird behavior. I was just wondering if we could do something about it. If we can't then that is fine, and I've just pointed out something unsafe :)

@thejoshwolfe
Copy link
Contributor

@Hejsil in the case where .ptr is null, is .len meaningful? If not, why not make the entire struct optional instead of just the one field?

@Hejsil
Copy link
Contributor Author

Hejsil commented Dec 15, 2018

Hmmmm. That's an idea. I just did "something"[0..].ptr

@Hejsil
Copy link
Contributor Author

Hejsil commented Dec 15, 2018

No, actually len is meaningful. It's the way in which the "feeder" knows how many bytes they need to consume and keep track of, so they need the len.

@Hejsil
Copy link
Contributor Author

Hejsil commented Dec 15, 2018

I could use a union(enum) though

@andrewrk
Copy link
Member

andrewrk commented Feb 10, 2019

Actually no, my example should be equal to this:

My mistake, you're right. This is an important issue which I'm glad you brought up. I'm still thinking about how to best resolve it, but I think it will be good to figure it out before the next release and document the intended semantics.

My first idea is that behavior should remain status quo, and we document how it is supposed to work. Which is that casting from *T to ?*T and vice-versa is always a no-op, which means if your *T is undefined, then your ?*T will be undefined as well. This would also be true for casting anyerror / ?anyerror, which now has the same 0 optimization as pointers.

I want to note that although in Zig, pointers cannot be null, they are allowed to have the address 0. Here's a Real Actual Use Case: https://github.com/andrewrk/clashos/blob/e55b6a26a5c01566da14b5f1306822c8da6fc978/src/main.zig#L81
This does mean that Zig programmers can technically abuse @ptrToInt and @intToPtr to get null pointers without going through optional, but the idea is that we've made optional pointers more convenient and a zero cost abstraction, and so people will use them. There is no limit to the abuse that can be done with @ptrToInt/@intToPtr and so it will always be with system programming languages.

@thejoshwolfe
Copy link
Contributor

Discussed this a bit with @andrewrk, and i feel confident about the plan:

  • *T zig pointers can be any value, even 0. See above linked usecase when this is useful.
  • ?*T optional zig pointers can be null which is represented as 0 in memory, and cannot point to address 0.
  • implicitly coercing a *T to a ?*T can cause safety checked undefined behavior in the case that the address is 0.

This fits in nicely with the rules in #1947.

@andrewrk
Copy link
Member

Related: #1952

@andrewrk
Copy link
Member

andrewrk commented Feb 14, 2019

When a pointer may not be address 0, an optional pointer shall have the same bit pattern. Therefore if a pointer which may not be address 0 has an undefined value (See #1947) the bit pattern may be regardless be 0, and when it is implicitly casted to an optional pointer, the optional pointer has an undefined value as well. However, implicit cast from T to ?T even if T is undefined is expected to produce a non-null value. So we have three choices:

Possible Solution 1. Implicit cast from ptr which may not be address 0 to optional pointer is not a no-op; if the value is 0 then zig chooses any non-zero bit pattern for the optional pointer. This is sub-optimal in terms of generated machine code, but avoids confusing semantics. Here's the LLVM it could codegen to:

  %1 = load i8*, i8** %x, align 8
  %2 = icmp neq i8* %1, i8* null
  %3 = zext %2 to i64
  %4 = or %3, %1
  store i8* %4, i8** %y, align 8

Possible Solution 2. Make it safety-checked undefined behavior to implicit cast undefined value of pointer which may not have address 0 to optional pointer. In other words, if your pointer is undefined or illegally address 0 and you try to cast it to an optional pointer, boom.

Possible Solution 3: Define T to ?T when T is a pointer which may not have address 0 to result in an unchanged bit pattern. This means implicit casting (*T)(undefined) to ?*T results in an undefined value.

@andrewrk andrewrk modified the milestones: 0.4.0, 0.5.0 Apr 8, 2019
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Sep 20, 2019
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Dec 31, 2019
@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 10, 2020
@SpexGuy
Copy link
Contributor

SpexGuy commented Nov 5, 2020

After some discussion, @andrewrk, @thejoshwolfe, and I came to the following conclusions: undefined is a viral value, like NaN. Operations on undefined values either produce undefined or manifest undefined behavior. Coercion from any value of type T to ?T constitutes an operation, so coercing any undefined non-optional value to an optional value either produces undefined or causes undefined behavior if the input value is undefined. This holds true for all types, not just pointers.

As a sanity check, we analyzed the behavior of std.HashMap under these rules when given an undefined value. We found that these rules imply that, if the value is undefined for a certain key, get returns an undefined optional. This seems fine, except that contains(k) is implemented as get(k) != null. However, changing the implementation of this function to getEntry(k) != null is more efficient code and does not suffer from the same problem. We found no other problems with undefined values in the type.

So we've decided to accept this rule and close this issue. It is superseded by #63 and #211, which cover adding debug checks for these cases where possible.

@Rocknest
Copy link
Contributor

Rocknest commented Dec 12, 2020

@SpexGuy but what about var data: ?[*]const u8 = ""[0..].ptr; its not obvious that an undefined value appears here.

Related: 435c8ad

@SpexGuy
Copy link
Contributor

SpexGuy commented Dec 12, 2020

String literals are null-terminated now, so that's not an empty slice anymore. The pointer is defined in this case. But in the case of an empty slice, the undefined value actually comes from converting a *[0]T to a slice, because zero sized pointers had no bits. Once #6706 is implemented, this footgun shoud disappear as well.

The other ways to create an empty slice are as follows:

So I don't think there's an easy way left to accidentally end up with an undefined slice pointer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants