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
cmd/compile: -d=checkptr doesn't detect invalid pointer fields in converted pointers-to-structs #36017
Comments
This works as intended. The rule 1 of unsafe.Pointer:
If you change to:
Then the compiler will report correctly. |
@cuonglm Yes, I'm aware that it will work if the struct contains the field directly. It may not break any rules for the root type, but it does break the same rules for the pointer type field. |
What do you mean "but it does break the same rules for the pointer type field"? In your example, |
@cuonglm Yes, both are pointers of the same size. However, if I understood correctly, |
Maybe you can read https://go-review.googlesource.com/c/go/+/162237 I quote the commit message here:
|
Sure, but again, it can be useful to check both rules for pointer fields as well (if the element types are not the same). Just to clarify: I'm not claiming it's a bug in |
But those struct fields are the same in your example, both fields are pointer type. So I don't get the point that you think it's invalid. |
OK, sorry, let me explain it a bit more carefully. Both examples execute the same operation (semantically): cast a pointer to a single The only difference is that the second example does the conversion by using a proxy struct type of the same size. It silently converts a So the point is not that In fact, the same code can be made valid by switching the type of |
No, they're total different. One cast from If you want to understand as using
There's no such operation, the conversion is done on the struct pointer, not their fields. |
You are right, there is no such explicit operation. However, there is implicit (or semantic, if you like) operation that happens during this If you run both tests, it will be clear that both are invalid semantically. Does it matter that much if there is no field conversion operation defined explicitly in this case? Both versions exhibit an invalid behavior as a matter of fact, so I'm trying to propose to introduce more (optional) runtime check(s) to detect those issues as well. |
I thought about doing this, but it would be very difficult in general. Because data structures can be cyclic, to be fully general, we'd have to test for graph isomorphism, which is NP complete. Maybe there's some sweet spot of partial testing that still helps but isn't intractable. (I thought I filed an issue for this, but I can't immediately find it. Maybe I realized the difficulty before even filing the issue.) |
@mdempsky Should the |
@mdempsky, why would we have to test for graph isomorphism? Why wouldn't it suffice to check that all pointers reachable by tracing the data structure from a typed root match the size and pointer layout of the type through which they are reached? |
@bcmills Yeah, I think graph isomorphism was too general of a problem to reference here. (In particular, graph isomorphism assumes edges are indistinguishable, but our edges are uniquely distinguishable by their memory offset within the origin node/variable.) I think fully and recursively tracing out all pointers to make sure they point to the right type would be sufficient. It would take at least time proportional to the amount of memory reachable from the converted pointer though, which could be substantial in some cases. That's also just for handling conversions involving numbers, pointers, and structs. It becomes even more complex if we want to worry about Go language types (channels, maps, interfaces, functions). |
Since this is more or less C/syscall-related, limiting the scope to numbers, pointers and structs should be sufficient. Also, the overhead is clear, but this would still be very useful for debugging unsafe code. I already found a few bugs in one complex codebase using @mdempsky I could help with the implementation, but I might have a few questions along the way. Can I contact you in the Gophers Slack or somewhere else? |
@dennwc Thanks. Questions related to the issue would be best asked and answered here. General questions about compiler internals would be best on golang-dev@. |
What version of Go are you using (
go version
)?What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
What did you expect to see?
Both
TestUnsafeCast
andTestUnsafeStructCast
fail withgo test -gcflags=-d=checkptr
, since both use invalidunsafe.Pointer
casts.What did you see instead?
Only the
TestUnsafeCast
fails. Pointer fields are not verified properly.The text was updated successfully, but these errors were encountered: