@dipinhora is there a test that can be put in for this? I'm good with merging, want to check if there is anything we can do to make sure we don't get a regression.

@SeanTAllen Not sure. I'll have to think about it. However, it's unlikely I'll have time before when you plan on doing the release (assuming this is to be included in the release).

@dipinhora merging then

This patch does not fix the issue as demonstrated by the following test.

class iso _TestStringReserveMinusOne is UnitTest
  """
  Test that we can use a value of -1 to reserve a string of
  the maximum length.
  """
  fun name(): String => "builtin/String.reserve"

  fun apply(h: TestHelper) =>
    let s = recover String end
    s.reserve(-1)

    var n: USize = 256

    while n > 0 do
      s.append(" ")
      n = n - 1
    end

    h.assert_eq[USize](s.space(), USize.max_value() - 1)
    h.assert_true(s.is_null_terminated())

I also mentioned that:

However, the fix shown below reveals a bigger issue here which is that pool_alloc_size(size) then fails to allocate a pointer (this time of the correct size) since there is no block which is large enough.

This is also not addressed by the patch.

@malthe when running @Theodus's minimal reproduction program:

actor Main
  new create(env:Env) =>
    let s = recover String end
    s.reserve(-1)
    // s.reserve(1_000_000_000) also works fine

    var n: USize = 129 // 128 is fine, 129 results in SEGFAULT

    while n > 0 do
      s.append(" ")
      n = n - 1
    end

    DoNotOptimise[String tag](s)
    DoNotOptimise.observe()

I get the following output with this bugfix:

vagrant@ubuntu-xenial:~/dhp$ ./t 
out of memory: : Cannot allocate memory
Aborted (core dumped)

Which is expected behavior since the OS cannot allocate the amount of memory requested (as you indicated).

As @Praetonus pointed out in #2013 (comment), we're currently not handling memory exhaustion and this is a case of memory exhaustion.

Without this bugfix, the issue was memory clobbering and segfaults due to memory clobbering. With this bug fix, it is appropriately memory exhaustion due to the large amount of memory being requested that the OS is unable to allocate.

OK I see. That makes sense, thanks for clarifying. I do worry that what I saw while debugging the issue is still a potential problem, which is that if there is no heap pool large enough for an allocation then it would just fail silently. There seem to be some preset size classes.

If you can craft a test case or a small program to reproduce what you saw during debugging it would help significantly in being able to fix things.

My understanding of the allocation logic is that it checks to see if a preset size class/heap pool can be (re)used. If not, it falls back to requesting the memory from the OS instead. In both cases, ponyint_virt_alloc should ensure that the application prints out out of memory: : Cannot allocate memory (or similar out of memory: <OS ERROR>) prior to aborting if the OS is unable to satisfy a memory allocation request.