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

propagate "uses reflection" through SSA stores #783

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

mvdan
Copy link
Member

@mvdan mvdan commented Aug 1, 2023

(see commit message)

Fixes #685.

Up until now, the new SSA reflection detection relied on call sites
to propagate which objects (named types, struct fields) used reflection.
For example, given the code:

    json.Marshal(new(T))

we would first record that json.Marshal calls reflect.TypeOf,
and then that the user's code called json.Marshal with the type *T.

However, this would not catch a slight variation on the above:

    var t T
    reflect.TypeOf(t)
    t.foo = struct{bar int}{}

Here, type T's fields such as "foo" and "bar" are not obfuscated,
since our logic sees the call site and marks the type T recursively.
However, the unnamed `struct{bar int}` type was still obfuscated,
causing errors such as:

    cannot use struct{uKGvcJvD24 int}{} (value of type struct{uKGvcJvD24 int}) as struct{bar int} value in assignment

The solution is to teach the analysis about *ssa.Store instructions
in a similar way to how it already knows about *ssa.Call instructions.
If we see a store where the destination type is marked for reflection,
then we mark the source type as well, fixing the bug above.

This fixes obfuscating github.com/gogo/protobuf/proto.
A number of other Go modules fail with similar errors,
and they look like very similar bugs,
but this particular fix doesn't apply to them.
Future incremental fixes will try to deal with those extra cases.

Fixes burrowers#685.
@mvdan mvdan requested review from lu4p and pagran August 1, 2023 23:13
Copy link
Member

@lu4p lu4p left a comment

Choose a reason for hiding this comment

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

I would've done this differently, but this also works.

@lu4p lu4p merged commit 23c8641 into burrowers:master Aug 2, 2023
5 checks passed
@mvdan mvdan deleted the reflect-anon branch September 23, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Build error and garble unsupport github.com/gogo/protobuf@v1.3.2
3 participants