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

Multiple issues in array data setter #153

Open
squizz617 opened this issue Feb 1, 2022 · 1 comment
Open

Multiple issues in array data setter #153

squizz617 opened this issue Feb 1, 2022 · 1 comment
Labels
help wanted Extra attention is needed

Comments

@squizz617
Copy link
Contributor

Bug report

Required Info:

  • Operating System:
    • Ubuntu 20.04
  • Installation type:
    • Binary installation via apt
  • Version or commit hash:
    • foxy
  • DDS implementation:
    • Fast-RTPS
  • Client library (if applicable):
    • N/A

Steps to reproduce issue

  1. Clone and build messages
$ mkdir -p ~/idltest_ws/src
$ cd ~/idltest_ws/src
$ git clone [email protected]:squizz617/idltest_msgs.git
$ cd ~/idltest_ws
$ colcon build --symlink-install
  1. Source workspaces (replace zsh with bash if using Bash)
$ source /opt/ros/foxy/setup.zsh
$ source ~/idltest_ws/install/setup.zsh
  1. Download and run PoC
$ wget https://gist.githubusercontent.com/squizz617/13631a18d25a1b7836414cce0c579227/raw/3e5113c78f2b1b7fe3296a2b29298afad6d8349e/poc_idl_array.py
$ chmod a+x poc_idl_array.py
$ ./poc_idl_array.py

Expected behavior

The PoC is self-explanatory. On the fixed arrays of built-in types, it tests the following three properties: (1) setting legitimate values should succeed, (2) setting out-of-valid-range values should fail, and (3) setting values of mismatching types should fail.

Actual behavior

The issues can be broadly categorized into three types.

  1. Missing range checks for numeric type (integer and float) arrays

uint8 array allows out of range integers to be assigned:

from idltest_msgs.msg import UInt8FixedArray
ui8fa = UInt8FixedArray()
ui8fa.data[0] = -1 # should fail but doesn't
print(ui8fa.data[0]) # value becomes 255
ui8fa.data[1] = 256 # should fail but doesn't
print(ui8fa.data[1]) # value becomes 0

int8 array also allows out of range integers to be assigned:

from idltest_msgs.msg import Int8FixedArray
i8fa = Int8FixedArray()
ui8fa.data[0] = -129 # should fail but doesn't
print(ui8fa.data[0]) # value becomes 127
ui8fa.data[1] = 128 # should fail but doesn't
print(ui8fa.data[1]) # value becomes -128

float32 array allows double to be assigned:

from idltest_msgs.msg import Float32FixedArray
f32fa = Float32FixedArray()
f32fa.data[0] = 1.0e+365 # should fail but doesn't
print(f32fa.data[0]) # value becomes inf
  1. Auto casting of data of wrong types

Integer arrays accept floats and casts them to int, dropping precision:

i8fa = Int8FixedArray()
i8fa.data[2] = 3.141592 # should fail but doesn't
print(i8fa.data[2]) # value becomes 3
i8fa.data[3] = 314.1592 # should fail but doesn't
print(i8fa.data[3]) # value becomes 58

For the types that cannot be casted to int, the assignment fails but the exception is raised not by the IDL:

i8fa.data[4] = "string" # fails (int() casting failure, not idl type check failure)
i8fa.data[5] = \x00 # fails (int() casting failure, not idl type check failure)
  1. Missing type checks for bool, byte, and string arrays

Any data of any type can be assigned to bool, byte or string arrays:

boolfa = BoolFixedArray()
bytefa = ByteFixedArray()
sfa = StringFixedArray()

# nothing fails
boolfa.data[0] = 32
boolfa.data[1] = 3.141592
boolfa.data[2] = 1.0e+365
boolfa.data[3] = 3.141592
boolfa.data[4] = "string"
boolfa.data[5] = [1, 2, 3]
boolfa.data[6] = {1: 2}
print(boolfa) # the illegitimate values are assigned as is

bytefa.data[0] = 32
bytefa.data[1] = 3.141592
bytefa.data[2] = 1.0e+365
bytefa.data[3] = 3.141592
bytefa.data[4] = "string"
bytefa.data[5] = [1, 2, 3]
bytefa.data[6] = {1: 2}
print(bytefa) # the illegitimate values are assigned as is

sfa.data[0] = 32
sfa.data[1] = 3.141592
sfa.data[2] = 1.0e+365
sfa.data[3] = 3.141592
sfa.data[4] = [1, 2, 3]
sfa.data[5] = {1: 2}
print(sfa) # the illegitimate values are assigned as is

In a nutshell, array elements are not properly checked at the time of assignment, unlike what's done for the built-in types (e.g., we cannot assign 256 or 3.14 to a uint8 variable, we cannot assign 3.14 to a string variable, ...).

@clalancette clalancette added the help wanted Extra attention is needed label Feb 17, 2022
@clalancette
Copy link
Contributor

Thanks for the report! Absolutely, this looks like an issue. We may not get to fixing it soon, but we are happy to review a pull request if you have some time to try and fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants