[YAML parser] Configurable maximum values during parsing

[mikaelarguedas]

This set of maximum values should be reevaluated:

rcl/rcl_yaml_param_parser/src/parser.c

Lines 65 to 71 in 5edf27b

	#define MAX_STRING_SIZE 128U
	#define PARAMS_KEY "ros__parameters"
	#define NODE_NS_SEPERATOR "/"
	#define PARAMETER_NS_SEPERATOR "."
	#define MAX_NUM_NODE_ENTRIES 256U
	#define MAX_NUM_PARAMS_PER_NODE 512U

[kekstee]

I was just about to open an issue about this set of limits.

More specifically I easily hit the 128 byte string length on resource file paths which will look something like:
"sharepath(absolute) + some subfolder + filename"

Maybe the length limit for keys and values should be separated?
Is this limit on strings enforced anywhere else?

[jacobperron]

We should make the maximum values for string size, number of nodes, and number of parameters configurable. Perhaps it makes sense to default all of them to "no limit" and users can configure a hard limit to avoid dynamic memory allocation.

[jdlangs]

@jacobperron could you provide some thoughts on what you consider configurable? Can we get away with compile-time definitions or is something more complex needed?

[jacobperron]

Compile-time definitions might be okay, but then I guess one would have to compile rcl_yaml_param_parser from source to change a value. This is inconvenient if you've installed ROS 2 binaries. It might be nice to have more flexibility and configure these parser parameters at runtime. For example, we could bootstrap parameter parsing by first parsing an optional YAML file containing the parser parameters.

It would be good to get other thoughts on the matter.

[clalancette]

Before we go too much further, I took a quick look at the code to make sure I could say something intelligent. If you look at the places that MAX_STRING_SIZE is used (here, here, here, and here), it doesn't actually prevent allocations from happening. It really just bounds the size that they could potentially be.

Given that, it is unclear to me what the purpose of the bound is. @anup-pem is the original author of the code that got merged as #235 . @anup-pem , could you comment on the reasons why you added the MAX_STRING_SIZE like you did, and some of the consequences of increasing that value and/or removing it altogether? Thanks in advance.

[jdlangs]

Given the impeding Eloquent feature freeze, is there any quick fix we can get in that would be accepted, especially if it's considered a breaking change? It would be really unfortunate if we ended up being stuck with this regression for another whole release cycle.

[dirk-thomas]

The planned freeze date for Eloquent is in two days. Given that there is not even a proposal at the moment I don't think it is realistic to expect any changes to land in time. (I anyone proves me wrong I am more than happy to see this improved.)

[clalancette]

I will point out that simply increasing the size is something we can do after the Eloquent release, as it shouldn't be an API or ABI break. I'd still like to hear from @anup-pem about the reasons for the original limitation and downsides to increasing it before making a decision here.

[jdlangs]

My proposal for a quick fix is to compile out the bounds check unless a compile definition is provided. Anyone relying on this bounding behavior (which I suspect is very few people) can build rcl_yaml_param_parser instead of using prebuilt binaries until a more comprehensive runtime configuration option can be proposed/designed.

[jdlangs]

Since no one spoke up against the compile definition idea, I've created a minimal PR for evaluation that implements the proposal.