Fix integer underflow error in hh_shared.c
commit7af7723cb456244039905d279dd42220418cc222
authorDwayne Reeves <dreeves@fb.com>
Fri, 26 Aug 2016 22:59:15 +0000 (26 15:59 -0700)
committerHhvm Bot <hhvm-bot-bot@fb.com>
Sat, 27 Aug 2016 02:03:10 +0000 (26 19:03 -0700)
treedb8bc6b5a60e320a731c5c58130746580906e2bf
parent139d1ea9aec5fa4e3d8c0d7dab251883a2c70551
Fix integer underflow error in hh_shared.c

Summary:
{D3452498} uses `hcounter` to keep track of the number of elements in the shared hashtable. This counter is incremented when we add an element and decremented when we remove.

At least that is how you would think it would work logically, but things are more complex than that.

* When we remove elements from the hashtable we keep the hash that his associated with the slot
* In `hh_add` if we are writing to a slot that is empty but has a hash assigned to it, then we won't increment `hcounter`
* `hh_move` can claim a new empty slot (with no hash previously assigned to it)

With this in mind `hcounter` can actually count down too far. For example if we do:

```
hh_add "0" "Foo"; // hcounter = 1 since we wrote to a new slot
hh_move "0" "1";  // hcounter = 1 even though we will take up a new slot
hh_add "0" "Bar"; // hcounter = 1 because we are reusing a slot
hh_remove "1";    // hcounter = 0 because we remove a slot
hh_move "0" "1";  // hcounter = 0 because we don't increment in hh_move
hh_add "0" "Bar"; // hcounter = 0 because we are reusing a slot
hh_remove "1";    // hcounter = -1 because we always decrement
```

This is more or less the operations Hack does during incremental type checking.

When `hcounter = -1` this causes an underflow because we treat it as an unsigned number which is really big so the next time we call `hh_add` we will think the hash table is full and throw an exception.

`hcounter` should really track the number of slots with hashes associated with them (nonempty). I added an additional check in `hh_move` to do this. However I changed our test to see if the hash table is full to not rely on `hcounter` because this is tricky to reason about. Instead we keep track of the initial slot we see and check each time we loop to see if we see that slot again. After one iteration over the hashed table we should see that initial slot again and then we raise the error.

I added this check to `find_slot` because we could potentially loop forever otherwise.

I tested this by adding a unit test which makes a very small hash table that I can test the edge cases on.

Reviewed By: gabelevi

Differential Revision: D3778949

fbshipit-source-id: 33d9fe0f21d33f0c9316849b0c046c810d26be01
hphp/hack/src/heap/hh_shared.c
hphp/hack/test/unit/heap/test_hashtbl.ml [new file with mode: 0644]