Skip to content

variable.c: Fix generic_fields_lookup_ensure_size to avoid use-after-free #13573

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

Closed
wants to merge 1 commit into from

Conversation

casperisfine
Copy link
Contributor

The following diff can cause a use-after-free:

diff --git a/variable.c b/variable.c
index a54bebcec0..53e01c1d60 100644
--- a/variable.c
+++ b/variable.c
@@ -1818,6 +1818,7 @@ generic_fields_lookup_ensure_size(st_data_t *k, st_data_t *v, st_data_t u, int e
         }

         fields_tbl = gen_fields_tbl_resize(fields_tbl, RSHAPE_CAPACITY(fields_lookup->shape_id));
+        rb_gc_start();
         *v = (st_data_t)fields_tbl;
     }
(2..).step(-1)

Since gen_fields_tbl_resize uses xrealloc, we might trigger GC after the old buffer has been freed, but the pointer in the generic ivar table hasn't been updated yet. So if the xrealloc trigger GC, we'll mark a freed buffer.

…r-free

The following diff can cause a use-after-free:

```c
diff --git a/variable.c b/variable.c
index a54bebc..53e01c1d60 100644
--- a/variable.c
+++ b/variable.c
@@ -1818,6 +1818,7 @@ generic_fields_lookup_ensure_size(st_data_t *k, st_data_t *v, st_data_t u, int e
         }

         fields_tbl = gen_fields_tbl_resize(fields_tbl, RSHAPE_CAPACITY(fields_lookup->shape_id));
+        rb_gc_start();
         *v = (st_data_t)fields_tbl;
     }
```

```ruby
(2..).step(-1)
```

Since `gen_fields_tbl_resize` uses `xrealloc`, we might trigger GC after the old
buffer has been freed, but the pointer in the generic ivar table hasn't been updated
yet. So if the `xrealloc` trigger GC, we'll mark a freed buffer.
@casperisfine
Copy link
Contributor Author

I think this bug is possible on 3.4.x

@casperisfine
Copy link
Contributor Author

Well, CI proves this wasn't the bug that cause the test_array.rb crash :/

@casperisfine
Copy link
Contributor Author

@peterzhu2118 tells me in xrealloc GC triggers before the malloc so GC shouldn't be able to trigger between generic_fields_lookup_ensure_size and the time we write the first attribute, except for when we grow from 0 to 3 capacity, but in that case it's not a problem.

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.

2 participants