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

Add check for unsafe conversions to atoms #570

Merged
merged 3 commits into from Oct 27, 2018

Conversation

Projects
None yet
2 participants
@seancribbs
Contributor

seancribbs commented Jul 30, 2018

Allowing input from unknown sources to be converted into atoms, if exploited, may cause the virtual machine to halt when the atom table overflows. Except in rare circumstances, one should avoid creating atoms at runtime for this reason, instead relying on conversion of input to existing atoms.

Creating an atom from a string or charlist should be done by using
String.to_existing_atom(string)

This comment has been minimized.

@rrrene

rrrene Jul 31, 2018

Owner

Since there is literally nothing else to criticise about this PR:

Can you indent the examples in @moduledoc with 4 spaces to be consistent with the other checks?

This comment has been minimized.

@seancribbs

seancribbs Jul 31, 2018

Contributor

😆 Of course!

This comment has been minimized.

@seancribbs

seancribbs Jul 31, 2018

Contributor

Addressed in a022d92.

@seancribbs

This comment has been minimized.

Contributor

seancribbs commented Jul 31, 2018

Slightly but not completely off-topic, I noticed this producing warnings for a few places in the credo codebase, but they were all at the module level. If you have any tips for how to avoid warning on values that will be computed at compile-time, I'd love to incorporate them (other than adding magic comments).

@rrrene

This comment has been minimized.

Owner

rrrene commented Aug 7, 2018

how to avoid warning on values that will be computed at compile-time

Puh, this is a tough one. We could certainly ignore values computed outside of regular functions (i.e. module attribute values, quote blocks, macros, etc.), but that would not fix all instances.

@seancribbs

This comment has been minimized.

Contributor

seancribbs commented Aug 7, 2018

We could certainly ignore values computed outside of regular functions

Yeah, after I typed the question I realized that in many cases, excluding macros wouldn't even be sufficient because it's totally fine (and expected) for them to define functions that themselves could create atoms at runtime. We should probably leave it up to the coder to determine whether to silence the warning in places that it is safe (compile-time). Given that, should I apply the magic comment to those safe places inside credo?

@seancribbs

This comment has been minimized.

Contributor

seancribbs commented Aug 22, 2018

@rrrene Would you review again?

@rrrene rrrene merged commit e894246 into rrrene:master Oct 27, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

rrrene added a commit that referenced this pull request Oct 27, 2018

@seancribbs

This comment has been minimized.

Contributor

seancribbs commented Oct 27, 2018

❤️ 💙 💚

@seancribbs seancribbs deleted the seancribbs:sdc/unsafe-atom-creation branch Oct 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment