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

Fix seg faults in SAJ parser #799

Merged
merged 1 commit into from Jul 29, 2022
Merged

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Jul 29, 2022

Previously if add_big_*() were called, Ruby code would seg fault:

-- C level backtrace information -------------------------------------------
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_vm_bugreport+0x561) [0x7ff955dd2da1] vm_dump.c:755
[0x7ff955bf5e61]
/opt/gitlab/embedded/lib/libruby.so.2.7(sigsegv+0x59) [0x7ff955d2eca9] signal.c:946
/lib/x86_64-linux-gnu/libc.so.6(0x7ff9559700c0) [0x7ff9559700c0]
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_id_table_lookup+0x7) [0x7ff955d6e727] symbol.h:72
/opt/gitlab/embedded/lib/libruby.so.2.7(lookup_method_table+0x14) [0x7ff955da6a5c] vm_method.c:188
/opt/gitlab/embedded/lib/libruby.so.2.7(method_entry_get) vm_method.c:747
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_callable_method_entry+0x20) [0x7ff955daea70] vm_method.c:891
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_call0+0x145) [0x7ff955dc5115] vm_eval.c:654
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_funcallv+0x38) [0x7ff955dc5c68] vm_eval.c:718
/opt/gitlab/embedded/lib/ruby/gems/2.7.0/gems/oj-3.13.18/lib/oj/oj.so(add_big_loc+0xac) [0x7ff9491815cc] saj2.c:300
/opt/gitlab/embedded/lib/ruby/gems/2.7.0/gems/oj-3.13.18/lib/oj/oj.so(calc_num+0xf0) [0x7ff949172e40] parser.c:531
/opt/gitlab/embedded/lib/ruby/gems/2.7.0/gems/oj-3.13.18/lib/oj/oj.so(parse+0x336) [0x7ff949174136] parser.c:713
/opt/gitlab/embedded/lib/ruby/gems/2.7.0/gems/oj-3.13.18/lib/oj/oj.so(parser_parse+0x9b) [0x7ff94917691b] parser.c:1344
[0x7ff955daaebd]

This appears to be happening due to improper pointer casting.

Previously if add_big_*() were called, Ruby code would seg fault:

```
-- C level backtrace information -------------------------------------------
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_vm_bugreport+0x561) [0x7ff955dd2da1] vm_dump.c:755
[0x7ff955bf5e61]
/opt/gitlab/embedded/lib/libruby.so.2.7(sigsegv+0x59) [0x7ff955d2eca9] signal.c:946
/lib/x86_64-linux-gnu/libc.so.6(0x7ff9559700c0) [0x7ff9559700c0]
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_id_table_lookup+0x7) [0x7ff955d6e727] symbol.h:72
/opt/gitlab/embedded/lib/libruby.so.2.7(lookup_method_table+0x14) [0x7ff955da6a5c] vm_method.c:188
/opt/gitlab/embedded/lib/libruby.so.2.7(method_entry_get) vm_method.c:747
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_callable_method_entry+0x20) [0x7ff955daea70] vm_method.c:891
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_call0+0x145) [0x7ff955dc5115] vm_eval.c:654
/opt/gitlab/embedded/lib/libruby.so.2.7(rb_funcallv+0x38) [0x7ff955dc5c68] vm_eval.c:718
/opt/gitlab/embedded/lib/ruby/gems/2.7.0/gems/oj-3.13.18/lib/oj/oj.so(add_big_loc+0xac) [0x7ff9491815cc] saj2.c:300
/opt/gitlab/embedded/lib/ruby/gems/2.7.0/gems/oj-3.13.18/lib/oj/oj.so(calc_num+0xf0) [0x7ff949172e40] parser.c:531
/opt/gitlab/embedded/lib/ruby/gems/2.7.0/gems/oj-3.13.18/lib/oj/oj.so(parse+0x336) [0x7ff949174136] parser.c:713
/opt/gitlab/embedded/lib/ruby/gems/2.7.0/gems/oj-3.13.18/lib/oj/oj.so(parser_parse+0x9b) [0x7ff94917691b] parser.c:1344
[0x7ff955daaebd]
```

This appears to be happening due to improper pointer casting.
@stanhu
Copy link
Contributor Author

stanhu commented Jul 29, 2022

I will look at writing a test later, though I don't fully understand yet when add_big_loc is called.

@ohler55
Copy link
Owner

ohler55 commented Jul 29, 2022

Thanks for the fix. That code would be exercised with the new parser when a large number is encountered. Something like 20 digits or so. I'll wait to merge if you want to write a test.

@stanhu
Copy link
Contributor Author

stanhu commented Jul 29, 2022

Yeah, that's what I thought. But in my test I'm not seeing the number yet. I'll keep digging.

Feel free to merge; I'll submit a follow-up pull request later.

@ohler55
Copy link
Owner

ohler55 commented Jul 29, 2022

Okay but I'll wait to release.

@ohler55 ohler55 merged commit 438361a into ohler55:develop Jul 29, 2022
@stanhu
Copy link
Contributor Author

stanhu commented Jul 29, 2022

Ok, I confirmed that these entries in the JSON payload were causing this crash:

{
     "y": [
          -11.899999999999999,
          -10.2,
          -8.5,
          -6.800000000000001
     ]
}

Though I'm not sure why this isn't crashing at the moment:

diff --git a/test/test_saj.rb b/test/test_saj.rb
index 806b22e..ad250fe 100755
--- a/test/test_saj.rb
+++ b/test/test_saj.rb
@@ -12,7 +12,7 @@ $json = %{{
       "string": "message",
       "hash"  : {
         "h2"  : {
-          "a" : [ 1, 2, 3 ]
+          "a" : [ 1, 2, 3, -11.899999999999999 ]
         }
       }
     }
@@ -164,6 +164,7 @@ class SajTest < Minitest::Test
                   [:add_value, 1, nil],
                   [:add_value, 2, nil],
                   [:add_value, 3, nil],
+                  [:add_value, -11.899999999999999, nil],
                   [:array_end, 'a'],
                   [:hash_end, 'h2'],
                   [:hash_end, 'hash'],

stanhu added a commit to stanhu/oj that referenced this pull request Jul 29, 2022
stanhu added a commit to stanhu/oj that referenced this pull request Jul 29, 2022
stanhu added a commit to stanhu/oj that referenced this pull request Jul 29, 2022
@stanhu
Copy link
Contributor Author

stanhu commented Jul 29, 2022

Ok, #800 should do the trick.

stanhu added a commit to stanhu/oj that referenced this pull request Jul 29, 2022
ohler55 pushed a commit that referenced this pull request Jul 29, 2022
stanhu added a commit to stanhu/oj that referenced this pull request Aug 17, 2022
stanhu added a commit to stanhu/oj that referenced this pull request Aug 17, 2022
This seg fault only occurred if the SAJ parser handler tracked
locations of hashes with Bignum values with inputs such as:

```json
      {
        "width": 192.33800000000002,
        "xaxis": {
          "anchor": "y"
        }
      }
```

This was missed in ohler55#799.
ohler55 pushed a commit that referenced this pull request Aug 18, 2022
This seg fault only occurred if the SAJ parser handler tracked
locations of hashes with Bignum values with inputs such as:

```json
      {
        "width": 192.33800000000002,
        "xaxis": {
          "anchor": "y"
        }
      }
```

This was missed in #799.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants