[Ruby] Message.decode/encode: Add max_recursion_depth option

[lfittl]

This allows increasing the recursing depth from the default of 64, by
setting the "max_recursion_depth" to the desired integer value. This is
useful to encode or decode complex nested protobuf messages that otherwise
error out with a RuntimeError or "Error occurred during parsing".

Fixes #1493

[haberman]

Please use UPB_DECODE_MAXDEPTH(). And we should also avoid overwriting the options completely, eg.

  options |= UPB_DECODE_MAXDEPTH(FIX2INT(depth));

[haberman]

Ditto here.

[haberman]

Looks like we need this implemented on the JRuby side too.

[lfittl]

@haberman Thanks for the review - addressed in the latest commit.

[lfittl]

Note this is changed to match the UPB library behavior (> => >=), and SINK_MAXIMUM_NESTING is incremented by one accordingly.

[lfittl]

@haberman I wanted to check if you could re-trigger a test build so we can confirm that the issues are now fixed? Thanks!

[lfittl]

@acozzette Thanks for the re-run - I can't really explain the test failure, it does not seem related to the actual code change. I've just rebased onto the latest main, in case that fixes the errors.

Could you re-run once more? Thank you!

[acozzette]

@lfittl I noticed that some of our Ruby tests started failing a few days ago, though I have not had a chance to investigate yet. It could well be that the test failure is unrelated.

[lfittl]

@lfittl I noticed that some of our Ruby tests started failing a few days ago, though I have not had a chance to investigate yet. It could well be that the test failure is unrelated.

Yeah, looks like the release tests are still failing (as they are on #9291), but otherwise this is green now (looks like the rebase fixed the other tests).

@haberman Let me know if you'd like to see any other adjustments. Thanks for reviewing!

[lfittl]

@haberman Happy New Year! - let me know if you'd like to see anything else adjusted on this PR :)

[acozzette]

I tried to resolve the merge conflicts in the GitHub UI just now. Hopefully that will work and then I will merge this unless @haberman has other comments.

[haberman]

I think the only remaining issue is making sure we are being as consistent as possible with other languages:

I just checked and Java, C++, and appears to calls this "recursion limit" instead of "max recursion depth:

protobuf/csharp/src/Google.Protobuf/CodedInputStream.cs

Lines 231 to 241 in 3f5fc4d

	/// <summary>
	/// Returns the recursion limit for this stream. This limit is applied whilst reading messages,
	/// to avoid maliciously-recursive data.
	/// </summary>
	/// <remarks>
	/// The default limit is 100.
	/// </remarks>
	/// <value>
	/// The recursion limit for this stream.
	/// </value>
	public int RecursionLimit { get { return state.recursionLimit; } }

protobuf/src/google/protobuf/io/coded_stream.h

Lines 418 to 419 in 3e1967e

	// Sets the maximum recursion depth. The default is 100.
	void SetRecursionLimit(int limit);

protobuf/java/core/src/main/java/com/google/protobuf/CodedInputStream.java

Lines 386 to 400 in d8ccfbf

	/**
	* Set the maximum message recursion depth. In order to prevent malicious messages from causing
	* stack overflows, {@code CodedInputStream} limits how deeply messages may be nested. The default
	* limit is 100.
	*
	* @return the old limit.
	*/
	public final int setRecursionLimit(final int limit) {
	if (limit < 0) {
	throw new IllegalArgumentException("Recursion limit cannot be negative: " + limit);
	}
	final int oldLimit = recursionLimit;
	recursionLimit = limit;
	return oldLimit;
	}

In that case, could we call the parameter recursion_limit instead of max_recursion_depth?

Sorry for the delay!

[acozzette]

Let me go ahead and merge this, and I will send out a follow-up pull request to rename the parameter to recursion_limit. Thanks, @lfittl!

[lfittl]

@acozzette @haberman Excellent, thanks for the help & merging this :)