Set func pointer to null in Closure __invoke

[morrisonlevi]

I've written the comment as if it's happened and been agreed to, but I want to have a little bit of discussion here. The only reason I can think of for this to not be done unconditionally, is something like this:

Detect detect use-after-frees in this code in non-debug asan builds, and avoid the absolutely minuscule cost of setting it to null in release builds. This means that we would want to be sure that literally everything in the engine and the entire ecosystem should know this is an invalid pointer even though it's not null.

For instance:

void (*og_zend_execute_internal)(zend_execute_data *execute_data, zval *return_value);
void my_extension_execute_internal(zend_execute_data *execute_data, zval *return_value) {
  og_zend_execute_internal(execute_data, return_value);
  // know that execute_data->func may be non-null but invalid.
}

That seems like a stretch. Of course, I did not know this and a customer hit this in production, causing a crash.

I think it would be best to set this to null, rather than requiring them to do something like:

void my_extension_execute_internal(zend_execute_data *execute_data, zval *return_value) {
  bool is_trampoline = execute_data->func
    && (execute_data->func->common.flags & ZEND_ACC_CALL_VIA_TRAMPOLINE) != 0;
  
  og_zend_execute_internal(execute_data, return_value);
  
  if (is_trampoline) {
    // know that you cannot touch execute_data->func
  }
}

Note that observers avoid this because the end observer is skipped in cases like this. Maybe @bwoebi can provide a more detailed explanation there.

Also note that even if we set this to null, there is still a potential gotcha here. I don't know how many people would expect execute_data->func to be null after the call when it wasn't before. The profiling extension I work on would have been okay, but I think this is already a decently big gotcha, no need to make it doubly-sharp by leaving an invalid, non-null pointer on the execute_data.

[derickr]

I think this is a good idea.