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

type-cosplay might detect issues within macros #16

Open
oslfmt opened this issue Aug 11, 2022 · 0 comments
Open

type-cosplay might detect issues within macros #16

oslfmt opened this issue Aug 11, 2022 · 0 comments

Comments

@oslfmt
Copy link
Contributor

oslfmt commented Aug 11, 2022

In the anchor check that is being added to type-cosplay in #15, I swap some logic around and first check the type X in X::deser(), before checking the function call. Later I check the function call. Thus, it the code catches any function calls and later filters them out.

Besides any optimization issues, I'm debating on moving the filter for code from expansions (expr.span.from_expansion()). The reason is because doing so seems like an arbitrary filtering decision (but I could be wrong) and we might be mindlessly just filtering out code that should be checked. (although at the same time, most of the filtered out code will be the code expanded from macros, and if a program is using a macro, the implementation details of the macro rests on the macro devs, not the people using it, so in that sense we should filter it out?).

Anyways, to get to my point, if we don't filter out macros, we may catch calls like X::call(), where X implements Discriminator, which is one of the types we're interested in. Case in point, when running on the insecure example, the lint will catch a type in the macro, IdlAccount. However, the associated function call on this type was try_deserialize, which is what we want, so the lint doesn't flag.

This raises an interesting question. Do we want to raise possible warnings for code the program should not be responsible, or not? Say the Anchor macro calls try_from_slice instead. This is insecure code, and from a security standpoint, should be flagged. But from a user-of-the-lint standpoint, it's bad ux since the user never typed this code out, so they have no idea what's going on. They'll probably be confused and have to do some digging. I'm not sure what best practice is, since I feel like lints should just focus on the user's code, but it feels wrong to just let potential bugs slip away?

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

1 participant