[Bug]: Segfault in dd_observe_fiber_switch

[cleverhoods]

Bug report

dd_observe_fiber_switch function in /tmp/pear/temp/datadog_trace/ext/handlers_fiber.c:112 does not check for ZEND_FIBER_STATUS_DEAD thus it can lead to a Segmentation fault.

PHP version

8.1

Tracer or profiler version

0.99.1

Installed extensions

No response

Output of phpinfo()

No response

Upgrading from

No response

[bwoebi]

Hey @cleverhoods,

Are you able to reproduce that somehow? I've tried, but fail to. I'm also not sure what special handling would need to happen here, if the fiber is dead? I also don't quite know from your report where exactly the invalid pointer dereference for the segfault then will happen?

Thanks!

[cleverhoods]

Hi @bwoebi,

The issue was originating from this method:

protected function replacePlaceholders(array &$elements) {
    if (!isset($elements['#attached']['placeholders']) || empty($elements['#attached']['placeholders'])) {
      return FALSE;
    }

    // The 'status messages' placeholder needs to be special cased, because it
    // depends on global state that can be modified when other placeholders are
    // being rendered: any code can add messages to render.
    // This violates the principle that each lazy builder must be able to render
    // itself in isolation, and therefore in any order. However, we cannot
    // change the way \Drupal\Core\Messenger\Messenger works in the Drupal 8
    // cycle. So we have to accommodate its special needs.
    // Allowing placeholders to be rendered in a particular order (in this case:
    // last) would violate this isolation principle. Thus a monopoly is granted
    // to this one special case, with this hard-coded solution.
    // @see \Drupal\Core\Render\Element\StatusMessages
    // @see https://www.drupal.org/node/2712935#comment-11368923

    // First render all placeholders except 'status messages' placeholders.
    $message_placeholders = [];
    $fibers = [];
    foreach ($elements['#attached']['placeholders'] as $placeholder => $placeholder_element) {
      if (isset($placeholder_element['#lazy_builder']) && $placeholder_element['#lazy_builder'][0] === 'Drupal\Core\Render\Element\StatusMessages::renderMessages') {
        $message_placeholders[] = $placeholder;
      }
      else {
        // Get the render array for the given placeholder
        $fibers[$placeholder] = new \Fiber(function () use ($placeholder_element) {
          return [$this->doRenderPlaceholder($placeholder_element), $placeholder_element];
        });
      }
    }
    $iterations = 0;
    while (count($fibers) > 0) {
      foreach ($fibers as $placeholder => $fiber) {
        if (!$fiber->isStarted()) {
          $fiber->start();
        }
        elseif ($fiber->isSuspended()) {
          $fiber->resume();
        }
        // If the Fiber hasn't terminated by this point, move onto the next
        // placeholder, we'll resume this fiber again when we get back here.
        if (!$fiber->isTerminated()) {
          // If we've gone through the placeholders once already, and they're
          // still not finished, then start to allow code higher up the stack to
          // get on with something else.
          if ($iterations) {
            $fiber = \Fiber::getCurrent();
            if ($fiber !== NULL) {
              $fiber->suspend();
            }
          }
          continue;
        }
        [$markup, $placeholder_element] = $fiber->getReturn();

        $elements = $this->doReplacePlaceholder($placeholder, $markup, $elements, $placeholder_element);
        unset($fibers[$placeholder]);
      }
      $iterations++;
    }

    // Then render 'status messages' placeholders.
    foreach ($message_placeholders as $message_placeholder) {
      $elements = $this->renderPlaceholder($message_placeholder, $elements);
    }

    return TRUE;
  }

The issue arises because the $placeholder variable from the initial foreach loop is reused in the subsequent foreach loop in the following while loop. This leads to corruption of the Fibers, as they may access inconsistent or overwritten state during execution.

I've created a ticket to have this fixed in Drupal (https://www.drupal.org/node/3490455) however the Datadog part is still valid IMO and checks for ZEND_FIBER_STATUS_DEAD would be great to be introduced.