Lock Not Released on Loop Early Exit

[Vivek5041]

Location: runtime/gc/concurrent-stack.c
Function: CC_stack_try_close()
Lines: 199-205
Bug Pattern: Lock acquired in loop, early break without unlock

The Code

bool CC_stack_try_close(CC_stack* stack, HM_chunkList removed) {

  // First, do a quick check to avoid taking locks unnecessarily
  bool allEmpty = TRUE;
  for (size_t i = 0; i < stack->numStacks; i++) {
    HM_chunkList thisBag = &(stack->stacks[i].storage);
    if (NULL != thisBag->firstChunk) {
      allEmpty = FALSE;
      break;
    }
  }

  /** We checked that all are empty; now just have to confirm and close. This
    * works by taking all the locks, verying that each is empty, and closing
    * it. If any has been extended in the meantime, we have to abort.
    */
  if (allEmpty) {
    size_t i;
    for (i = 0; i < stack->numStacks; i++) {
      pthread_mutex_lock(&(stack->stacks[i].mutex));  // Line 200: Lock acquired
      HM_chunkList thisBag = &(stack->stacks[i].storage);
      if (NULL != thisBag->firstChunk)
        break;  // Line 203: BUG - breaks WITHOUT unlocking lock i!
      stack->stacks[i].isClosed = TRUE;
    }

    if (i == stack->numStacks) {
      // success! unlock everything and return.
      stack->allClosed = TRUE;
      for (size_t j = 0; j < i; j++) {
        pthread_mutex_unlock(&(stack->stacks[j].mutex));
      }
      return TRUE;
    }

    // Otherwise, we failed to close. Unlock everything before continuing.
    for (size_t j = 0; j < i; j++) {  // Line 217: Only unlocks 0..i-1
      pthread_mutex_unlock(&(stack->stacks[j].mutex));
    }
    // BUG: Lock i is still held!
  }

  /** If we reach here, then there is at least one bag which is non-empty,
    * and we're currently holding no locks. Proceed by taking a batch of
    * elements and returning them.
    */

  for (size_t i = 0; i < stack->numStacks; i++) {
    HM_chunkList thisBag = &(stack->stacks[i].storage);
    if (NULL != HM_getChunkListFirstChunk(thisBag)) {
      pthread_mutex_lock(&(stack->stacks[i].mutex));
      HM_chunk chunk = HM_getChunkListFirstChunk(thisBag);
      HM_unlinkChunk(thisBag, chunk);
      pthread_mutex_unlock(&(stack->stacks[i].mutex));

      HM_appendChunk(removed, chunk);
    }
  }

  return FALSE;
}

The Problem

1. Line 200: pthread_mutex_lock(&(stack->stacks[i].mutex)) - Lock i acquired
2. Line 202: Check finds that thisBag->firstChunk != NULL
3. Line 203: Loop exits via break - Lock i is NOT released
4. Line 217: Unlock loop only releases locks 0 through i-1
5. Result: Lock i remains held indefinitely

Why It's a Bug

When the condition NULL != thisBag->firstChunk is true after acquiring lock i:

• The break statement exits the loop immediately
• Lock i acquired at line 200 is NEVER released
• The unlock loop at lines 217-219 only releases locks 0..i-1 (not lock i)
• Lock i remains held until process termination
• Any future attempt to acquire lock i will block forever

[shwestrick]

Thanks for finding this; seems like an easy fix -- just an off-by-one error on this line (seems like it should be j <= i):

mpl/runtime/gc/concurrent-stack.c

Line 217 in a71659b

for (size_t j = 0; j < i; j++) {

It would be good to do some testing to confirm. This code-path isn't easy to trigger, because it involves entanglement management.

[shwestrick]

It occurs to me also that this function can "partially close" the CC_stack, by setting isClosed on some of the per-processor stacks, but not all. This might be correct, but I don't immediately see why at the moment. Seems worth revisiting.