opentelemetry-php: [opentelemetry-php-instrumentation] - Segfault when increasing arguments in pre hook for native functions/methods

PHP 8.2.7 with latest opentelemetry 1.0.0beta6.

Steps to reproduce Given this code where we modify the arguments will crash with a segfault.

<?php
OpenTelemetry\Instrumentation\hook(
    null,
    'array_slice',
    pre: function(null $instance, array $params) {
        return [$params[0], $params[1], 1];
    },
    post: function(null $instance, array $params, $returnValue) {
    }
);

var_dump(array_slice([1,2,3], 1));

This will not crash:

     null,
     'array_slice',
     pre: function(null $instance, array $params) {
-        return [$params[0], $params[1], 1];
+        return [$params[0], $params[1]];
     },
     post: function(null $instance, array $params, $returnValue) {
     }

This will also not crash:

     'array_slice',
     pre: function(null $instance, array $params) {
         return [$params[0], $params[1], 1];
-    },
-    post: function(null $instance, array $params, $returnValue) {
     }
 );

It will also not crash if the function is a userland function, so it only crashes if the function is defined in a PHP extension (core or PECL). I originally discovered the crash while working on instrumentation for PECL amqp, specifically when hooking into AMQPExchange::publish and expanding the args there.

About this issue

  • Original URL
  • State: closed
  • Created a year ago
  • Comments: 19 (3 by maintainers)

Most upvoted comments

@lstrojny I’ve created a new issue for tracking this as an enhancement request rather than a bug: https://github.com/open-telemetry/opentelemetry-php/issues/1223

Since the main concern with this issue (based on the title) was the crash and that is now fixed, it makes sense to handle the feature request in a separate issue.

You can add a short comment there that one desired use case for that feature would specifically be AMQPExchange::publish instrumentation, so it would be clear that the issue is actually relevant right now.

I spent some time on this today. No fixes yet, but it seems to be related to garbage collection: the exception is in php shutdown. Whilst playing around, I also came across another failure scenario: we can’t return $params from a pre callback. The two may be related, as they’re both to do with ref counting for garbage collection, I think. open-telemetry/opentelemetry-php-instrumentation#71