-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
Error Using Garble with github.com/lucas-clemente/quic-go Library #466
Comments
Could be related to #410; I seem to recall it also involves embedded structs, and the error looks similar. |
Update: this is not a duplicate, because #410 is fixed in master whereas I can still reproduce here. |
I was finally able to pinpoint what's causing this. It's our handling of embedded type aliases; because the typechecker doesn't record when type aliases are used, we have to do our best to reverse-engineer when they are used and how, and that code is complex and buggy. The best solution would be for the go/types API to just make that information readily available; I've left a comment at golang/go#44410 (comment) to bring that up. Until then, I'll see if we can make our workaround a bit smarter to cover this particular edge case. Our current workaround code gets confused because there's a |
There are two scenarios when it comes to embedding fields. The first is easy, and we always handled it well: type Named struct { Foo int } type T struct { Named } In this scenario, T ends up with an embedded field named "Named", and a promoted field named "Foo". Then there's the form with a type alias: type Named struct { Foo int } type Alias = Named type T struct { Alias } This case is different: T ends up with an embedded field named "Alias", and a promoted field named "Foo". Note how the field gets its name from the referenced type, even if said type is just an alias to another type. This poses two problems. First, we must obfuscate the field T.Alias as the name "Alias", and not as the name "Named" that the alias points to. Second, we must be careful of cases where Named and Alias are declared in different packages, as they will obfuscate the same name differently. Both of those problems compounded in the reported issue. The actual reason is that quic-go has a type alias in the form of: type ConnectionState = qtls.ConnectionState In other words, the entire problem boils down to a type alias which points to a named type in a different package, where both types share the same name. For example: package parent import "parent/p1" type T struct { p1.SameName } [...] package p1 import "parent/p2" type SameName = p2.SameName [...] package p2 type SameName struct { Foo int } This broke garble because we had a heuristic to detect when an embedded field was a type alias: // Instead, detect such a "foreign alias embed". // If we embed a final named type, // but the field name does not match its name, // then it must have been done via an alias. // We dig out the alias's TypeName via locateForeignAlias. if named.Obj().Name() != node.Name { As the reader can deduce, this heuristic would incorrectly assume that the snippet above does not embed a type alias, when in fact it does. When obfuscating the field T.SameName, which uses a type alias, we would correctly obfuscate the name "SameName", but we would incorrectly obfuscate it with the package p2, not p1. This would then result in build errors. To fix this problem for good, we need to get rid of the heuristic. Instead, we now mimic what was done for KnownCannotObfuscate, but for embedded fields which use type aliases. KnownEmbeddedAliasFields is now filled for each package and stored in the cache as part of cachedOutput. We can then detect the "embedded alias" case reliably, even when the field is declared in an imported package. On the plus side, we get to remove locateForeignAlias. We also add a couple of TODOs to record further improvements. Finally, add a test. Fixes burrowers#466.
There are two scenarios when it comes to embedding fields. The first is easy, and we always handled it well: type Named struct { Foo int } type T struct { Named } In this scenario, T ends up with an embedded field named "Named", and a promoted field named "Foo". Then there's the form with a type alias: type Named struct { Foo int } type Alias = Named type T struct { Alias } This case is different: T ends up with an embedded field named "Alias", and a promoted field named "Foo". Note how the field gets its name from the referenced type, even if said type is just an alias to another type. This poses two problems. First, we must obfuscate the field T.Alias as the name "Alias", and not as the name "Named" that the alias points to. Second, we must be careful of cases where Named and Alias are declared in different packages, as they will obfuscate the same name differently. Both of those problems compounded in the reported issue. The actual reason is that quic-go has a type alias in the form of: type ConnectionState = qtls.ConnectionState In other words, the entire problem boils down to a type alias which points to a named type in a different package, where both types share the same name. For example: package parent import "parent/p1" type T struct { p1.SameName } [...] package p1 import "parent/p2" type SameName = p2.SameName [...] package p2 type SameName struct { Foo int } This broke garble because we had a heuristic to detect when an embedded field was a type alias: // Instead, detect such a "foreign alias embed". // If we embed a final named type, // but the field name does not match its name, // then it must have been done via an alias. // We dig out the alias's TypeName via locateForeignAlias. if named.Obj().Name() != node.Name { As the reader can deduce, this heuristic would incorrectly assume that the snippet above does not embed a type alias, when in fact it does. When obfuscating the field T.SameName, which uses a type alias, we would correctly obfuscate the name "SameName", but we would incorrectly obfuscate it with the package p2, not p1. This would then result in build errors. To fix this problem for good, we need to get rid of the heuristic. Instead, we now mimic what was done for KnownCannotObfuscate, but for embedded fields which use type aliases. KnownEmbeddedAliasFields is now filled for each package and stored in the cache as part of cachedOutput. We can then detect the "embedded alias" case reliably, even when the field is declared in an imported package. On the plus side, we get to remove locateForeignAlias. We also add a couple of TODOs to record further improvements. Finally, add a test. Fixes #466.
What version of Garble and Go are you using?
What environment are you running Garble on?
go env
OutputWhat did you do?
What did you expect to see?
Expected Garble to complete without error. If I exclude
github.com/lucas-clemente/quic-go/
from GOGARBLE, Garble will complete without error.What did you see instead?
Received an error for the
github.com/lucas-clemente/quic-go/
repository which importsConnectionState
fromgithub.com/marten-seemann/
The trail of structures looks like:
Which imports from https://github.com/marten-seemann/qtls-go1-17/blob/master/common.go
Which uses embedded structure
The text was updated successfully, but these errors were encountered: