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

use ast.literal_eval instead of eval #73

Merged
merged 3 commits into from
Mar 21, 2018

Conversation

akio
Copy link
Contributor

@akio akio commented Jan 11, 2018

This PR adds stricter syntax checking to bool constant value parsing.
Constant value for bool is rarely used in the wild (There are no usage in all released ROS packages named -msgs or -srvs).
But the current implementation allow to execute any Python code in the right side from = in bool constant lines with some tricks. I thinks this could be a potential security concern.

Bool constant syntax in this commit based on [ROS2 IDL] (http://design.ros2.org/articles/interface_definition.html).
This change introduce backward incompatibility since it limits acceptable bool literal syntaxs significantly.
But as I mentioned above, this syntax is rarely used and IMHO there is virtually no bad effect in real.
Considerable more moderate approach is to use literal_eval() function in Python's ast module.

@dirk-thomas
Copy link
Member

Since we can't assume that no code is using this I would rather not introduce such an unnecessary backward incompatibility. Especially since this package uses the same branch for all ROS distros. Please change the patch to use ast.literal_eval() as you suggested to keep the behavior changes minimal.

@akio
Copy link
Contributor Author

akio commented Feb 28, 2018

Changed to use ast.literal_eval().

@@ -181,8 +182,7 @@ def convert_constant_value(field_type, val):
raise InvalidMsgSpec("cannot coerce [%s] to %s (out of bounds)"%(val, field_type))
return val
elif field_type == 'bool':
# TODO: need to nail down constant spec for bool
return True if eval(val) else False
return ast.literal_eval(val)
Copy link
Member

Choose a reason for hiding this comment

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

TL;DR I think that the logic for ensuring the return of a boolean should be kept here to preserve backward compatibility, aka return True if ast.literal_eval(val) else False.

Details: The new logic will make this function return numbers (integers or float) instead of python bool in the case where users provide numbers as a default.
For example the following msg file:

bool FOO=42
bool BAR=0
bool BLA=42.42
bool BLABLA=-42.42

currently results in (generated python message):

FOO = True
BAR = False
BLA = True
BLABLA = True

but with this PR will result in :

FOO = 42
BAR = 0
BLA = 42.42
BLABLA = -42.42

@akio
Copy link
Contributor Author

akio commented Mar 11, 2018

Considering @mikaelarguedas 's comment, I updated the eval line to return True/False value.

@mikaelarguedas
Copy link
Member

thanks @akio for the follow-up 👍 lgtm.
@dirk-thomas Does the change looks good to you too?

@dirk-thomas dirk-thomas changed the title Make syntax check for bool constant stricter use ast.literal_eval instead of eval Mar 21, 2018
@dirk-thomas
Copy link
Member

Thank you for the patch and for iterating on it.

@dirk-thomas dirk-thomas merged commit 3cea1fe into ros:indigo-devel Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants