Fix off by one errors in ocamlpool malloc
commit08b717c3a9c976a24182271c4cb9c9ff47a8ef49
authorThomas Jiang <thomasjiang@fb.com>
Tue, 1 Sep 2020 19:19:59 +0000 (1 12:19 -0700)
committerFacebook GitHub Bot <facebook-github-bot@users.noreply.github.com>
Tue, 1 Sep 2020 19:22:01 +0000 (1 12:22 -0700)
tree8de4f9e925d6d013a2746e6b0ae7971e93e63be8
parent9c298600069da3bd3029d75bc81b9c27aa8bcb53
Fix off by one errors in ocamlpool malloc

Summary:
I believe that there are two off by one issues in ocamlpool.c logic that causes a diff changing an abnormally large file to have a 2/4096 [Page_size] chance of causing an invariant violation and crashing. (Each issue on its own adds 1/4096 chance of crashing).

1. Double accounting for header when setting `ocamlpool_limit`
- Note: In addition to potentially crashing, this issue has the effect of potentially causing us to waste memory in our arena allocation scheme
2. Not mallocing enough space for ocaml's header

## Enough context to understand the issues.

### Headers

OCaml objects have a header struct. On our machines, this is currently 8 bytes. Thus, when we request a block of memory via `caml_alloc_for_heap`, 8 bytes of this is taken up by the header. So if we ask for an arena of size X words, we can only use (X - 1) words.

When rust asks ocamlpool.c for some memory via `ocamlpool_reserve_block`, we need to add our own header (which mimics the ocaml header). So if rust asks us for a block of 9 words, we will actually account for a 10 block chunk of the arena.

### Bug 1: Double accounting for header when setting `ocamlpool_limit`

Originally, we have this line of code
```
ocamlpool_limit = (value*)ocamlpool_root + 1;
```
`root` here is the start of our allocated arena. The intention of this line is to say, the first word of the arena is actually the header that ocaml uses. Thus, we can't use it.

However, this is double accounting for the header because before this line, we have already adjusted root to account for the header in the line
```
ocamlpool_root = (value)((value*)block + 1);
```
Thus, we are double accounting for the header and potentially wasting bytes.

### Bug 2: Not mallocing enough space for ocaml's header

When we run out of space in the arena and attempt to allocate a new arena, we ask ocaml for the number of words we want. So if rust wants 2000000 words worth of memory, we ask ocaml for 2000001 words worth of memory. We added 1 page to account for the header that we allocate for ourselves.
```
size_t size = words + 1;
...
ocamlpool_next_chunk_size = size;
```
However, these 2000001 words do not account for the fact that ocaml needs a page for its header. We should actually be asking for 2000002 words worth of memory! One page for ocaml's header. One page for our header. And 2000000 to give back to rust. So we actually need to be doing:
```
size_t size = words + 1;
...
ocamlpool_next_chunk_size = size + 1;
```

## Notes:

### You still haven't explained why we crash!
- We crash because our invariants are wrong. Let's say rust asks us for 2 million words of memory. We ask ocaml for 2000001 words of memory. We remember to account for ocaml header in `ocamlpool_root = (value)((value*)block + 1);` and then account for it again in `ocamlpool_limit = (value*)ocamlpool_root + 1;`. So now we think that we only have 1999999 words of memory left. How can we possibly fit the 2000001 words needed for the 2000000 words for rust and the 1 word for our own header?

### Why didn't we run into this error before?
There are two reasons why we didn't encounter this error reproducibly before. First, this error requires two different things to happen.
- First, we need to attempt to allocate more than the default arena size. This is because if rust attempts to allocate less than the default memory size, we will allocate an arena of more space than needed and we can cleanly fit the rust memory block into the arena. Thus, we'll usually only run into this bug when dealing with large allocations, such as those in a notorious map file.
- Second, under the hood of `caml_alloc_for_heap` is a call to `caml_aligned_malloc`, which sometimes actually allocates more memory than we request. This is so that our memory can be aligned to a page. Thus, unless our allocation is exactly the right size to be page aligned, (by my estimate, happens 1/4096 [Page_size] * 2 [for each off by one bug] of the time), we will always get back more memory than we need and therefore will not run into this bug.

Reviewed By: dabek

Differential Revision: D23441623

fbshipit-source-id: e4171ac81916bafafb96310590be534dfcac6fee
hphp/hack/src/utils/ocamlpool/ocamlpool.c