Skip to content
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.

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

Bug 64070 fix, traits aliasing of collided methods #277

Closed
wants to merge 3 commits into from

Conversation

nikita2206
Copy link
Contributor

Here's a description https://bugs.php.net/bug.php?id=64070

Basically the problem can be described so:
We have a trait Foo that has a method bar()
We have a trait Bar that has a method bar()
Bar uses Foo, and to avoid collisions, it aliases Foo::bar() as foo()

Now the problem: when Foo::bar() copied to a Bar with alias foo(), its function_name (zend_function.common.function_name) remains as "bar". So when some other class or trait uses Bar, it receives Foo::bar() as bar, not as foo() and that causes a problem.

@@ -3988,6 +3988,12 @@ static int zend_traits_copy_functions(zend_function *fn TSRMLS_DC, int num_args,
&& alias->trait_method->mname_len == fnname_len
&& (zend_binary_strcasecmp(alias->trait_method->method_name, alias->trait_method->mname_len, fn->common.function_name, fnname_len) == 0)) {
fn_copy = *fn;

/* Switch the name of the aliased function to the alias so that derived classes will get alias and not original name */
int alias_len = strlen(alias->alias);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable declarations have to happen at the top of the scope (C89). Also this should use alias->alias_len. Also I think you forgot to alloc/copy the NUL terminator.

Maybe just write fn_copy.common.function_name = estrndup(alias->alias, alias->alias_len) instead?

Apart from that, can't say whether patch is right or not. Don't know traits well enough.

@nikita2206
Copy link
Contributor Author

@nikic thanks, corrected.

@nikic
Copy link
Member

nikic commented Feb 18, 2013

@nikita2206 Why did you choose to use estrdup over estrndup? I don't know whether it makes a difference in this particular case, but I'd always use the estrndup variant if the structure provides you with the length.

@nikita2206
Copy link
Contributor Author

@nikic fixed

@nikita2206
Copy link
Contributor Author

@zend-dev Oh, I see, didn't check what was in hash_key. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants