Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed bug #61998 (Crash when declare trait after class if user define the same name as aliased one) #83

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

reeze commented May 15, 2012

Thanks ron for your test script. I've make a minimal reproducible one below:

In class Class1:

  • newFunc was referred to T::func
  • func was itself (by overriding);

In trait T

  • func was referred by Class1 and T itself;

--- since class was destroyed by reverse order --

  1. Destroy T: will not release the function name defined in trait. since
    the Class1 referred to this function.
  2. Destroy Class1:it will destroy the alias name since the aliased
    function name was referred to it.(this let riginal function name
    in trait unreleased and leak).
    after destroy function table it will destroy alias info. but alias name was
    already destroyed in function table releasing phrase. This cause double free(crash).

Solutions:

  1. Copy the whole function will solve the problem. but it was too heavy.
  2. Don't change the aliases function's name, since function call are always lookup by hash key name.
    but it will make reflection unhappy and can't throw right error message for function.
  3. Make a reference in function table if trait function was overrided to avoid releasing problem.
    This need to change reflection ignore it.get_defined_functions() & get_delcared_clesses()
    use this trick to filter special entry. so we need to change ReflectionClass::getMethods().

in summary I prefer option 3. What do you think?

<?php
class Class1 {
    use T {
        func as newFunc;
    }

    
public function func() { // <------------ if this override trait method and the method get aliased will lead crash
echo "From Class1::func\n";
}
}
    
class Class2 {
use T;
}
    
trait T { // <------------------------------ declare after the Class1 and it will be destroy before Class1
public function func() {
echo "From trait T\n";
}
}

Contributor

reeze commented May 15, 2012

@laruence thanks I will update that later.

It used to add a null to the start of the key to get a special key can't be accessed by user. the tmp_fn_name was emalloced with an additional byte to hold it.
please refer: Zend/zend_builtin_functions.c: ZEND_FUNCTION(create_function), I do it like that:)

On Tue, May 15, 2012 at 12:30 PM, Reeze Xia
reply@reply.github.com
wrote:

Thanks ron for your test script. I've make a minimal reproducible one below:

In class Class1:
newFunc was referred to T::func
func was itself (by overriding);

In trait T
func was referred by T and itself;

--- since class was destroyed by reverse order --

  1. Destroy T: will not release the function name defined in trait. since
      the Class1 referred to this function.
  2. Destroy Class1:it will destroy the alias name since the aliased
      function name was referred to it.(this let riginal function name
      in trait unreleased and leak).
      after destroy function table it will destroy alias info. but alias name was
      already destroyed in function table releasing phrase. This cause double free(crash).

Solutions:

  1. Copy the whole function will solve the problem. but it was too heavy.
  2. Don't change the aliases function's name, since function call are always lookup by hash key name.
       but it will make reflection unhappy and can't throw right error message for function.
  3. Make a reference in function table if trait function was overrided to avoid releasing problem.
       This need to change reflection ignore it.get_defined_functions() & get_delcared_clesses()
       use this trick to filter special entry. so we need to change ReflectionClass::getMethods().
    Hi:

thanks for your working on it. it is appreciated.

However, the trick way should not be encouraged, and after deeply look
into it, I got another option. I will work it out this weekend.

thanks

in summary I prefer option 3.  What do you think?

Laruence  Xinchen Hui
http://www.laruence.com/

Owner

laruence commented May 19, 2012

Hi:

my patch was attached here:
https://bugs.php.net/patch-display.php?bug_id=61998&patch=bug61998.patch&revision=latest

thanks

On Wed, May 16, 2012 at 2:24 PM, Account for PHP Pull Requests
reply@reply.github.com
wrote:

On Tue, May 15, 2012 at 12:30 PM, Reeze Xia
reply@reply.github.com
wrote:

Thanks ron for your test script. I've make a minimal reproducible one below:

In class Class1:
newFunc was referred to T::func
func was itself (by overriding);

In trait T
func was referred by T and itself;

--- since class was destroyed by reverse order --

  1. Destroy T: will not release the function name defined in trait. since
      the Class1 referred to this function.
  2. Destroy Class1:it will destroy the alias name since the aliased
      function name was referred to it.(this let riginal function name
      in trait unreleased and leak).
      after destroy function table it will destroy alias info. but alias name was
      already destroyed in function table releasing phrase. This cause double free(crash).

Solutions:

  1. Copy the whole function will solve the problem. but it was too heavy.
  2. Don't change the aliases function's name, since function call are always lookup by hash key name.
       but it will make reflection unhappy and can't throw right error message for function.
  3. Make a reference in function table if trait function was overrided to avoid releasing problem.
       This need to change reflection ignore it.get_defined_functions() & get_delcared_clesses()
       use this trick to filter special entry. so we need to change ReflectionClass::getMethods().
    Hi:

thanks for your working on it.  it is appreciated.

However, the trick way should not be encouraged, and after deeply look
into it,  I got another option.  I will work it out this weekend.

thanks

in summary I prefer option 3.  What do you think?

Laruence  Xinchen Hui
http://www.laruence.com/


Reply to this email directly or view it on GitHub:
#83 (comment)

惠新宸        laruence
Senior PHP Engineer
http://www.laruence.com

Contributor

reeze commented May 19, 2012

Add extra function to function_table is not that perfect.@laruence 's patch looks better.
closed

@reeze reeze closed this May 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment