Skip to content

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

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

reeze
Copy link
Contributor

@reeze 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";
}
}

@reeze
Copy link
Contributor Author

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:)

@php-pulls
Copy link

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/

@laruence
Copy link
Member

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

@reeze
Copy link
Contributor Author

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
@dunglas dunglas mentioned this pull request Sep 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reeze @php-pulls @laruence