brk() implementation is racy

[juj]

Emscripten moved to implement brk() in C instead of JS, with implementation

int brk(intptr_t ptr) {
  intptr_t last = (intptr_t)sbrk(0);
  if (sbrk(ptr - last) == (void*)-1) {
    return -1;
  }
  return 0;
}

However the implementation can race in multithreaded setting and produce an incorrect sbrk bump.

Suppose heap size is 2MB, and thread A is calling brk(4MB). last will come out at 2MB, so thread A is about to call sbrk(4MB-2MB) to increase the heap by two megabytes, but let's suppose it pauses there before the call.

Meanwhile, suppose thread B is also calling brk(4MB) to set the heap size to the same size. It computes that it should bump the size by 2MB, so it should also call sbrk(4MB-2MB). Now both thread A and thread B will be calling to bump break limit up by 2 megabytes. They both succeed, and heap will grow from 2MB -> 4MB -> 6MB. But both threads A and B had initially issued a call brk(4MB) to set the heap limit to the same 4MB ceiling!

I am not running into this issue in practice, but just noticed this when glancing over the implemented C code.

[kripken]

Good point, we made sbrk threadsafe but not brk, it looks like.

Worth fixing, but in most cases brk is not called directly, and both malloc and sbrk are safe, so low-risk. But I opened #10008 to show a clear error in a pthreads build.

[Albirair]

Shouldn't brk now be considered thread-safe considering that it only invokes sbrk which has been made thread-safe since commit d15f2 ? Thus, the only modification needed is removing the #if directive on condition EMSCRIPTEN_PTHREADS .

[kripken]

@Albirair I think it is still not thread-safe. sbrk itself is, but in brk we call sbrk twice, and there is a possibility for races because of that.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.