[Ruby] Message.decode/encode: Add max_recursion_depth option by lfittl · Pull Request #9218 · protocolbuffers/protobuf

lfittl · 2021-11-13T07:15:50Z

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

Depends on protocolbuffers/protobuf#9218, until that is merged this can only be used with a custom built protobuf gem. Fixes #209

haberman · 2021-11-24T02:15:08Z

+    VALUE depth = rb_hash_lookup(hash_args, ID2SYM(rb_intern("max_recursion_depth")));
+
+    if (depth != Qnil && TYPE(depth) == T_FIXNUM) {
+      options = FIX2INT(depth) << 16;


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

options |= UPB_DECODE_MAXDEPTH(FIX2INT(depth));

haberman · 2021-11-24T02:15:35Z

+    VALUE depth = rb_hash_lookup(hash_args, ID2SYM(rb_intern("max_recursion_depth")));
+
+    if (depth != Qnil && TYPE(depth) == T_FIXNUM) {
+      options = FIX2INT(depth) << 16;


Ditto here.

haberman · 2021-11-24T02:16:22Z

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

lfittl · 2021-11-26T06:14:57Z

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

lfittl · 2021-11-26T06:17:20Z

-    protected DynamicMessage build(ThreadContext context, int depth) {
-        if (depth > SINK_MAXIMUM_NESTING) {
+    protected DynamicMessage build(ThreadContext context, int depth, int maxRecursionDepth) {
+        if (depth >= maxRecursionDepth) {


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

lfittl · 2021-12-07T22:38:57Z

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

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 protocolbuffers#1493

lfittl · 2021-12-09T02:47:38Z

@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 · 2021-12-09T18:39:43Z

@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 · 2021-12-09T19:13:31Z

@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 · 2022-01-07T22:49:24Z

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

acozzette · 2022-02-03T16:19:59Z

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 · 2022-02-03T18:10:59Z

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 · 2022-02-09T16:44:06Z

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 · 2022-02-09T20:36:37Z

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

google-cla Bot added the cla: yes label Nov 13, 2021

lfittl added a commit to pganalyze/pg_query that referenced this pull request Nov 13, 2021

PgQuery.parse: Support complex queries with deeply nested ASTs

74d75f4

Depends on protocolbuffers/protobuf#9218, until that is merged this can only be used with a custom built protobuf gem. Fixes #209

lfittl mentioned this pull request Nov 13, 2021

PgQuery.parse: Support complex queries with deeply nested ASTs pganalyze/pg_query#238

Merged

lfittl added a commit to pganalyze/pg_query that referenced this pull request Nov 13, 2021

PgQuery.parse: Support complex queries with deeply nested ASTs

c60464a

Depends on protocolbuffers/protobuf#9218, until that is merged this can only be used with a custom built protobuf gem. Fixes #209

lfittl mentioned this pull request Nov 13, 2021

Failed to parse tree: Error occurred during parsing pganalyze/pg_query#209

Closed

lfittl added a commit to pganalyze/pg_query that referenced this pull request Nov 13, 2021

PgQuery.parse: Support complex queries with deeply nested ASTs

f98eab4

Depends on protocolbuffers/protobuf#9218, until that is merged this can only be used with a custom built protobuf gem. Fixes #209

lfittl force-pushed the issue-1493-add-decode-depth-option branch from c1d3b1b to fab7498 Compare November 14, 2021 16:59

lfittl changed the title ~~[Ruby] Message.decode: Add max_recursion_depth option~~ [Ruby] Message.decode/encode: Add max_recursion_depth option Nov 14, 2021

elharo requested a review from haberman November 15, 2021 14:52

elharo added ruby enhancement kokoro:force-run labels Nov 15, 2021

protobuf-kokoro removed kokoro:run labels Nov 15, 2021

haberman reviewed Nov 24, 2021

View reviewed changes

lfittl commented Nov 26, 2021

View reviewed changes

acozzette added kokoro:run labels Dec 7, 2021

protobuf-kokoro removed the kokoro:run label Dec 7, 2021

lfittl added 2 commits December 8, 2021 18:46

Address review comments

29909eb

lfittl force-pushed the issue-1493-add-decode-depth-option branch from 51433c5 to 29909eb Compare December 9, 2021 02:47

acozzette added the kokoro:run label Dec 9, 2021

protobuf-kokoro removed the kokoro:run label Dec 9, 2021

Merge branch 'master' into issue-1493-add-decode-depth-option

fda11da

acozzette added the kokoro:run label Feb 3, 2022

protobuf-kokoro removed the kokoro:run label Feb 3, 2022

acozzette added the wait for user action label Feb 4, 2022

acozzette merged commit fbe6ab2 into protocolbuffers:master Feb 9, 2022

lfittl deleted the issue-1493-add-decode-depth-option branch February 9, 2022 20:36

Conversation

lfittl commented Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haberman Nov 24, 2021

Choose a reason for hiding this comment

Uh oh!

haberman Nov 24, 2021

Choose a reason for hiding this comment

Uh oh!

haberman commented Nov 24, 2021

Uh oh!

lfittl commented Nov 26, 2021

Uh oh!

lfittl Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfittl commented Dec 7, 2021

Uh oh!

lfittl commented Dec 9, 2021

Uh oh!

acozzette commented Dec 9, 2021

Uh oh!

lfittl commented Dec 9, 2021

Uh oh!

lfittl commented Jan 7, 2022

Uh oh!

acozzette commented Feb 3, 2022

Uh oh!

haberman commented Feb 3, 2022

Uh oh!

acozzette commented Feb 9, 2022

Uh oh!

lfittl commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lfittl commented Nov 13, 2021 •

edited

Loading

lfittl Nov 26, 2021 •

edited

Loading