Input validation as a metric for quality

Quality Assurance
1

Security and robustness is a major concern to a lot of folks in ROS community. Keeping this in mind, I propose that sanitation and validation of inputs (and possibly outputs) be considered as a strong metric for calculating the quality of a package. Here are some of my thoughts:

In order to increase security and robustness, as a first step, we can start with checking inputs from

  • Subscribers and service servers
  • Command line
  • Parameter server

Some kinds of data require sanitation because they are constrained however, those constraints aren’t explicit such as range (input to switch case, pixel values, euler angles), mathematical relationships (quaternions), internal structure (URL, compressed octree maps) among others.

Ensuring the constraints are met in the input is important because of the anyone-can-publish nature of topics and parameters. The lack of good command line argument parsing tools for C++ (an important language in the community) and the allure of using command line for passing arguments can result in an unstable configuration. As a result, some packages are only a key-stroke away from either crashing or worse propagating the error onwards to other packages.

As a community, we should create packages that can handle erroneous inputs gracefully by

  • checking range of inputs either against constants (defined in the message)
  • using robust libraries like OpenCV which guarantee trimming of input to within acceptable ranges
  • using mathematical constraints, such as by normalizing quaternions on input
  • range checking while creating data-structures from compressed messages
  • using compatible libraries for dealing with URL (1 Each library has a different quirk in URL parsing)

I expect URL parsing to mostly be an issue in the near future with the increase in packages providing nodes to communicate over the internet [2]. However, others are a concern right now, especially during debugging. We can send erroneous input from command line tools which offer no validation (eg: default values for quaternion are [0, 0, 0, 0]).

The effect on performance, code complexity, development effort is expected to be minimum with an assured increase in overall robustness of the software stack. Moreover, a lot of packages already adhere to the best practices like this.

I’d like to confess in advance that I don’t know how to enforce or detect violations automatically, or even what all would constitute a violation of input validation since it might be a non-exhaustive list. Having said that, I’m eager to know your views on the topic and to answer, clarify and discuss.

Thanks
Kunal Tyagi

[2]: By then, hopefully, the issues are resolved in the upstream libraries

2 Likes
2

There is some discussion related to input validation here:

3

In the meanwhile I summarized considerations about “Design by Contract” (which relates to input and output validation, could also be used for consistency checking on the node/nodelet level) and how it could be applied to ROS in one of my fun projects rosdbc - README.md. It didn’t reach “implementation phase”. Don’t assume any progress in implementation… I decided to retire that project and continue with a project in ROS2 whenever I find some time and motivation :slightly_smiling_face: However the projects README.md is probably a shorter read than the thread.

4

The thread hyperlinked already points to a project where Python hypothesis is used for property based testing in ROS1. That would be a possible way to verify violations.

It could be even easier to integrate with ROS2 because the ROS2 infrastructure supports pytest out of the box with ament_cmake_pytest. Python hypothesis integrates with pytest out-of-the-box. If you search in the hypothesis readthedocs you will find out how it integrates with pytest. (BTW: hypothesis-pytest is legacy…)

5

Having an DBC sounds the best way forward, however, for a lot of languages, libraries/standard don’t exist to implement packages in such a fashion. I think with ROS2, we’ll be seeing packages in a number of different languages (at least Go, Rust, and C apart from Python, C++ and Lisp).

As such, I have a few open ended questions. How can we

  • score a given implementation (say on a scale of 0 being no contract to 5 being a contract with no assumptions left unchecked).
  • handle trade-offs: Lots of package owners might face the issue of increase in latency in order to increase their score from say 4 to 5. As a result, we might want to categorize types of assumptions so the highest priority ones affect score more than one with a low priority.
  • create guidelines regarding what a contract must have or better (next point)
  • create a general guideline for all languages which stays applicable with/without a DBC support. This will enable a C package to adhere to the guidelines as easily as a Python package
  • find out the major issues in integration between packages as a starting point for the previous points (some issues faced by me have been mentioned in passing but I’m by no means a good representation of ROS community)

Since DBC implementations are immature, and we should start somewhere so as to enable simpler on-boarding of packages to DBC paradigm. I like the idea of using Python hypothesis to create simple test nodes for other packages to run and test against.

I think for all major ROS packages with a message, we should add Python nodes to read and send messages (pre-defined types) to test the assumptions for that message. This helps people test if their system is resilient to wrong inputs by writing output tests themselves (following examples is easier than doing it from scratch).

Maybe make this a standard practices so people can call relevant functions from the message package to test the assumptions themselves for their messages. (This might result in the same code being written multiple times in multiple language or writing the code in C for inter-operability)

This also enables a debug launch method where people can launch these nodes to check if their packages are sending the correct data during normal run-time operations or run these nodes against a rosbag.

6

I don’t know if scoring is reasonable.

  1. In many cases input/output data validity cannot be defined on the ROS node level and the ROS library level. It depends on the overall application context. If e.g. data input/output to a ROS node/nodelet is considered valid or not depends in many cases on the application the node/nodelet is used in and cannot be defined in the node/nodelet scope in a generic way. Consider a node wrapping an IMU. The data types are simple (sensor_msgs - imu). If e.g. the accelerations are beyond the hardware specification it is pretty obvious that the output (e.g. published topic) is invalid. An application level node/nodelet which uses the IMU nodes data as input could validate it. For a generic node which uses the IMU nodes data as input, lets say to stabilize the pose of a drone, the “validity” of the input would depend on the overall application setup and capabilities (IMU node, “stabilization” node, motor control node, etc.). The acceleration input would be considered invalid above levels which would lead the overall setup to fail in stabilizing the drone pose.
  2. In many cases the interface data types are too complex to define what “valid” data is. E.g. consider a node/nodelet providing or consuming the sensor_msgs - PointCloud2 data type…
7

You want DbC checking enabled in development/integration versions of the software only (not in production versions). E.g. compiled programming languages with built-in support implement this with conditional “injection” of the DbC logic during compile time. “By throwing a compiler switch, Contracts code can be enabled or can be withdrawn from the compiled code.” (D’s Contract Programming vs C++'s). C++17 supports DbC as well (Support for contract based programming in C++). I don’t know if it is implemented in C++ compilers already and how. (If the language does not have built-in support the “manual” conditional “injection” of DbC logic is a potential source for errors. This applies on the ROS level as well.)

1 Like
8
  • See comment about “scoring” above. I think it is just possible to recommend what a contract should have (if reasonable).
9

I feel that a package with the correct checks on input and output would increase the robustness of any system it’s used in. I really like knowing that none of my nodes can seg fault or show undefined behavior, no matter what you throw at them. This is hard and time consuming. In order to incentivize such practices in the community, we need to reward the packages which aspire for a higher standard for stability as well as inform the wider community about them.

The Build: Passing tag in the README of a repository gives me a little more confidence in a foreign codebase. A Coverage: 90% tag increases my confidence but a Coverage: 30% doesn’t. A well-documented README (or similar file) ensures me that the developer wants other to use the package and has documented it’s correct usage and failings. These anecdotal feelings without any fact make me think that a score for robustness is reasonable.

Robustness is an abstract concept, but is influenced by a lot of factors. Resilience to random inputs (fuzzing) is one of them. Lack of seg-faults is another. There are a variety of factors, each with a different weight to a different segment of user. First step in increasing robustness is always ensuring you only accept the correct input and deliver the correct output. As a result, the first step towards a badge of robustness would be to have a badge related to Input and Output validation. It can be a simple Uses contracts: Yes/No sticker or a Seg-faults: Yes/No sticker or something a more complicated than that (if that’s possible).

10

I would be using Contracts to mean something to validate input/output against.

I completely agree with this. It’s upto the user to choose the correct contract. Contracts can’t be one-size-fits-all. Moreover, there is little reason not to parametrise the contracts. User might want to choose a lower and upper bound on acceleration along x-axis and y-axis and a total acceleration magnitude constraint. We can’t know it in advance. However, we can know that for sensor_msgs/NavSatFix.position_covariance_type, there are only a fixed range allowed, and this can be an example of standard contract. User might include a run time check (only option if the node is in say, python) or a compile time check. We shouldn’t limit the options either way. Providing contracts only to a certain language would change the capabilities of ROS as well as the results people expect in different languages.

I think contracts would be like messages. ROS would provide a few standard contracts, but users would be free to combine a standard contract provided by ROS as they wish or write their own to suit their needs. Just like how users might use the Imu message or want to create a new Imu message without orientation or a new message with pressure.

I’ve seen people use modified asserts and logging methods which activate based on the compile type (Debug, Release, ReleaseWithDebugInfo). There’s no reason why it shoudn’t be similar for contracts. This is what makes me skeptical of the contract proposal for C++.

The user should be able to use the contract how ever required. It could be compile time or run time.
Compile time would require using extra libraries or creating new macros or wait for future standard support. But this approach isn’t language agnostic. For run time support, leveraging DDS might be a good option.

On a related note, C++17 standard doesn’t have contracts.

This is the issue with not only many sensor_msgs (among other messages from visualization_msgs, etc.), but also configuration parameters and dynparams for nodes. This doesn’t mean we can’t do something for the simpler messages or simpler checks.

We can add simple contracts, specially the conditions ones written as comments in the messages (number of points is width*height. It’s a straight path to segmentation fault in Python as well as C++ without this simple check). In fact, the initial contracts would be simply whatever checks people already use.

I personally feel contracts shouldn’t rely solely on compile time DbC because it restricts people from using a language the rest of the logic would be best written in.

Sidenote: Any form of implemented contracts would need to be integrated with rostopic pub command line interface or with the message type itself. Or else the user might face trouble diagnosing the reason the messages (from command line) aren’t doing what they are supposed to do, and hunting down contracts in every subscribing node might be a headache. If contracts are good, but tooling doesn’t exist, people wouldn’t use them. If they are bad (the interest in DbC would like to say otherwise :slight_smile: ), they might as well not exist in the library.

11

To detect seg faults one usually runs the production code with a dynamic analysis tool (like e.g. valgrind/memcheck, clang AddressSanitizer consider googles advanced documentation for the AddressSanitizer or clang LeakSanitizer (googles advanced documentation for the LeakSanitizer).

To check against seg faults on the ROS level you would have to check if there is any response from the node (the test runner should not crash) after throwing some input at it. To generate as much as possible different input (for fuzzy testing) you would need some ROS level property based testing framework.

pyros-dev uses property based testing here (hypothesis_example.py) and here (test_basic_fields.py) to generate input data for tests. The same approach could be used to generate input data for ROS nodes. However the ROS1 test frameworks would need (significant) modification/extension to get the data into test cases. (In case I am wrong please correct me!)

12

Thanks for pyros. It looks like it can be the base of “fuzzy testing”.

We should decouple pyros-dev capabilities depending on which package the message belongs to (right now the core code is tightly coupled with std_msgs). It’d make it easier for people to write tests (parametrised) for their custom messages