Fix is_null_terminated reading arbitrary memory (issue #1425)

[Perelandric]

When _alloc is equal to _size, the is_null_terminated method will
point to arbitrary memory when checking for the 0 byte. This PR makes
that method first check that _alloc != _size before reading the
_size byte of the Pointer[U8].

Fixes #1425

[Perelandric]

It seems like there's an assumption in the library (at least sometimes) that the first _alloc byte that is greater than _size should be set to 0.

If this is consistent, then perhaps is_null_terminated can actually drop the (_ptr._apply(_size) == 0) check.

[jemc]

Thank you! Can you also add a test case that highlights the fixed case?

[Perelandric]

@jemc Tests added, though it occurs to me now that instead of this:

(_alloc > 0) and (_alloc != _size) and (_ptr._apply(_size) == 0)

we could probably do this:

(_alloc != _size) and (_ptr._apply(_size) == 0)

...since when they're not equal, _alloc should always be the greater of the two. I'll change it if it's deemed worthwhile and safe.

[Perelandric]

Appveyor has failed, travis is still running. The tests passed on my Linux box.

I changed the test case to no longer use a power of two length array. I don't know why that would make a difference, unless an array literal automatically allocates to the next power of two.

I'll add the power of two array test back in to see if it passes.

---

EDIT:

So it appears that an array literal does pre-allocate up to (presumably) the next power of two when it is not itself a power of two size. The tests have been updated to reflect this.

---

EDIT:

Well jeepers, this test fails no matter what I do...

    h.assert_true(String.from_iso_array(recover
      ['a', 'b', 'c']
    end).is_null_terminated())

Looks like I'm just born to lose. I'm going to reverse it back to h.assert_false and see what happens this time.

[Perelandric]

Well shucks. is_null_terminated returns neither true nor false for the same test.

**** FAILED: builtin/String.is_null_terminated
C:\projects\ponyc\packages\builtin_test\_test.pony:439: Assert true failed. 

**** FAILED: builtin/String.is_null_terminated
C:\projects\ponyc\packages\builtin_test\_test.pony:439: Assert false failed. 

I'm going to remove that test to see if it passes with only the one test. The test is in my comment above for quick reference.

[Praetonus]

The test is inconsistent because allocated memory isn't set to zero in release mode.

[Perelandric]

Ah, I was going to ask if that preallocation was zeroed.

So then the only way that's going to work is if the first pre-allocated bit gets zeroed consistently. I have no idea how to do that for the literals. (EDIT: Oh wait, that's array literals. Nevermind.) Looks like that's missing in from_iso_array as well, which would've handled this case.

[Perelandric]

PR #1436 sets the null byte in String.from_iso_array, so if that gets merged, I'll merge in that commit (if that's the right strategy) and redo the tests here.

[jemc]

It's been merged. Thanks!

[Perelandric]

Thanks @jemc, looks like everything passes.