use ast.literal_eval instead of eval #73
Conversation
|
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 |
|
Changed to use |
src/genmsg/msg_loader.py
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
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
|
Considering @mikaelarguedas 's comment, I updated the eval line to return True/False value. |
|
thanks @akio for the follow-up 👍 lgtm. |
|
Thank you for the patch and for iterating on it. |
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
-msgsor-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'sastmodule.