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

Question about suspicious code #13472

Closed
kov-serg opened this issue Aug 28, 2018 · 17 comments
Closed

Question about suspicious code #13472

kov-serg opened this issue Aug 28, 2018 · 17 comments
Labels
duplicate Duplicate issue. The duplicate issue is referenced in the comments

Comments

@kov-serg
Copy link

Expected and Actual Behavior

See no "Fatal error: Access to undeclared static property: Phalcon\Di::$_default" under high load.

Code to reproduce

<?php \Phalcon\Di::reset();

Question

Why in source of phalcon https://raw.githubusercontent.com/phalcon/cphalcon/master/build/php5/64bits/phalcon.zep.c

in line 7225:

if (UNEXPECTED(zend_hash_quick_find(&ce->properties_info, property_name, property_name_len + 1, hash_value, (void **) &temp_property_info)==FAILURE)) {

used &ce->properties_info instead of CE_STATIC_MEMBERS(ce) ?
And why

zend_update_class_constants(ce TSRMLS_CC);

placed after this line. Should it paced before line with zend_hash_quick_find?

Can any one explain this code?

  • Phalcon version: 2.0.8(no problem) -> 2.0.10+(bug)
  • PHP Version: 5.3.29(no problem) -> 5.4.45+(bug)
  • Operating System: any
  • Installation type: any
  • Server: Apache
@kov-serg
Copy link
Author

Another code to reproduce https://gist.github.com/Ultimater/4525404c2b48f6d7458b1ca2607d2fb8 problem. Line with

$di = new \Phalcon\Di\FactoryDefault();

could be changed to

\Phalcon\Di::reset();

result will be the same

@Jurigag
Copy link
Contributor

Jurigag commented Sep 7, 2018

As far as i know this issue happens on TS php.

@kov-serg
Copy link
Author

kov-serg commented Sep 7, 2018

I have this issue on jino.ru hosting. If I set 2.0.8+PHP5.3 - it works, 2.0.10+PHP5.4..5.6 - stable reproducible bug.
The only difference in source code in this place.
...
#if PHP_VERSION_ID < 50400
property = zend_std_get_static_property(scope, name, name_length, 0 TSRMLS_CC);
#else
property = zephir_std_get_static_property(scope, name, name_length, zend_inline_hash_func(name, name_length + 1), 0, property_info TSRMLS_CC);
#endif
...
And the only difference in zend_std_get_static_property and zephir_std_get_static_property in two places I shown before.

@kov-serg
Copy link
Author

kov-serg commented Sep 7, 2018

And there used NTS versions of PHP

@Jeckerson
Copy link
Member

What OS?

@kov-serg
Copy link
Author

$ cat /etc/centos-release
CentOS Linux release 7.5.1804 (Core)
$ uname -a
Linux srv66-h-st.jino.ru 3.10.0-862.9.1.el7.x86_64 #1 SMP Mon Jul 16 16:29:36 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
$cat /proc/cpuinfo|grep name|head -1
model name : Intel(R) Xeon(R) CPU X3470 @ 2.93GHz

@Jurigag
Copy link
Contributor

Jurigag commented Sep 18, 2018

@kov-serg this C code is generated by zephir if you think we should change it or do anything else write here please - https://github.com/phalcon/zephir or feel free to create PR about this if you know hot to fix it. I will close this issue because there is already one similar existing #12056 but about C code we should discuss it in zephir repository what is generated and fix it if there is something wrong

Here is discussion about this issue in zephir zephir-lang/zephir#1530

@Jurigag Jurigag closed this as completed Sep 18, 2018
@Jurigag Jurigag added the duplicate Duplicate issue. The duplicate issue is referenced in the comments label Sep 18, 2018
@hakimio
Copy link

hakimio commented Oct 4, 2018

@kov-serg
Have you tried to compile zephir with your changes to object.c#L1275@zephir ? Does it fix the "Access to undeclared static property" error?

@hakimio
Copy link

hakimio commented Oct 4, 2018

@sjinks Do you know the reason for the suspicious code?

@kov-serg
Copy link
Author

kov-serg commented Oct 8, 2018

Because of this:
http://php53.sewandenjoy.com/bug.php - works
http://php54.sewandenjoy.com/bug.php - problem
http://php55.sewandenjoy.com/bug.php - problem
http://php56.sewandenjoy.com/bug.php - problem

I found only difference in source code of phalcon.
https://raw.githubusercontent.com/phalcon/cphalcon/phalcon-v2.0.8/build/64bits/phalcon.zep.c
near line: "static zval **zephir_std_get_static_property("

#if PHP_VERSION_ID < 50400
	property = zend_std_get_static_property(scope, name, name_length, 0 TSRMLS_CC);
#else
	property = zephir_std_get_static_property(scope, name, name_length, zend_inline_hash_func(name, name_length + 1), 0, property_info TSRMLS_CC);
#endif

This code generated by zephir. But when I do this zephir forum was down.

@hakimio
Copy link

hakimio commented Oct 8, 2018

@kov-serg you are looking at some old phalcon version which was made with an old zephir version. That check was removed from zephir some time ago.

Right now you only have the following:

But when I do this zephir forum was down.

What do you mean? Do what? Zephir forum?

@kov-serg
Copy link
Author

kov-serg commented Oct 8, 2018

Yes. I unable to open zephir forum in august.
I know this define was removed. I've compare source code - code is exactly the same except several lines. That's why I asking about them. I still unable to reproduce problem on virtual machine with exactly same versions of os,apache,php and phalcon, may be I need same cpu to reproduce it, but hosting I use unable to solve this issue for more then a year.

@hakimio
Copy link

hakimio commented Oct 8, 2018

You should be able to easily reproduce the issue with Thread-Safe (the most important part) PHP 7 on Windows and Apache web server. Once you have everything set-up, just have some loop which makes asynchronous calls (curl?) to your phalcon endpoint and I can guarantee that it will crash.

@hakimio
Copy link

hakimio commented Oct 8, 2018

Also, Zephir forum works well for me. Are you behind Russian national firewall or something?

@kov-serg
Copy link
Author

kov-serg commented Oct 8, 2018

Now it works. But in august and september it was inaccessible even trough tor.
There is NTS build at jino. http://php56.sewandenjoy.com/info/
PHP Extension Build | API20131226,NTS
Thread Safety | disabled
Phalcon Version | 2.0.10
Build Date | Jul 3 2018 15:31:35
Powered by Zephir | Version 0.9.2a-dev

@hakimio
Copy link

hakimio commented Oct 9, 2018

The bug should not be reproducable with NTS version.

@kov-serg
Copy link
Author

kov-serg commented Oct 9, 2018

In theory there is no difference between theory and practice. In practice there is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Duplicate issue. The duplicate issue is referenced in the comments
Projects
None yet
Development

No branches or pull requests

4 participants