Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Re: [PHP-CVS] com php-src: Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding config): NEWS Zend/zend.c

Posted by Dmitry Stogov 
Hi Laruence,

Thank you for sending this.

I'm not sure if the patch is completely correct.
With the patch all the threads share the single copy of
script_encoding_list and when one thread terminates it calls
compiler_globals_dtor() and frees the script_encoding_list. But other
threads still keep reference to it.

I think we have to duplicate script_encoding_list for each thread in the
same way as we do for CG(function_table).

Also I noticed a related issue. At zend.c compiler_globals_dtor()
CG(script_encoding_list) deallocated using free() and in
zend_multibyte.c zend_multibyte_set_script_encoding() using efree().

I suppose the second place has to be fixed.

I would appreciate if you could look into the problems.

Thanks. Dmitry.


On 05/03/2012 06:51 PM, Laruence wrote:
> Hi, Dmitry:
>
> you may want to review this, :)
>
> thanks
> On Thu, May 3, 2012 at 10:39 PM, Xinchen Hui<[email protected]> wrote:
>> Commit: 72f19e9a8bcf5712b24fa333a26616eff19ac1ce
>> Author: Xinchen Hui<[email protected]> Thu, 3 May 2012 22:39:53 +0800
>> Parents: d74d88fbb9c29b1dd5ff05a54b72cf7c9250955c
>> Branches: PHP-5.4
>>
>> Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=72f19e9a8bcf5712b24fa333a26616eff19ac1ce
>>
>> Log:
>> Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding config)
>>
>> Bugs:
>> https://bugs.php.net/61922
>>
>> Changed paths:
>> M NEWS
>> M Zend/zend.c
>>
>>
>> Diff:
>> diff --git a/NEWS b/NEWS
>> index 8796cf4..9ef6abf 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -10,6 +10,8 @@ PHP NEWS
>> (Laruence)
>>
>> - Core:
>> + . Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding config).
>> + (Laruence)
>> . Fixed missing bound check in iptcparse(). (chris at chiappa.net)
>> . Fixed bug #61827 (incorrect \e processing on Windows) (Anatoliy)
>> . Fixed bug #61761 ('Overriding' a private static method with a different
>> diff --git a/Zend/zend.c b/Zend/zend.c
>> index dd299f1..37a1a27 100644
>> --- a/Zend/zend.c
>> +++ b/Zend/zend.c
>> @@ -781,6 +781,8 @@ void zend_register_standard_ini_entries(TSRMLS_D) /* {{{ */
>> void zend_post_startup(TSRMLS_D) /* {{{ */
>> {
>> #ifdef ZTS
>> + zend_encoding **script_encoding_list;
>> +
>> zend_compiler_globals *compiler_globals = ts_resource(compiler_globals_id);
>> zend_executor_globals *executor_globals = ts_resource(executor_globals_id);
>>
>> @@ -795,7 +797,12 @@ void zend_post_startup(TSRMLS_D) /* {{{ */
>> zend_destroy_rsrc_list(&EG(persistent_list) TSRMLS_CC);
>> free(compiler_globals->function_table);
>> free(compiler_globals->class_table);
>> - compiler_globals_ctor(compiler_globals, tsrm_ls);
>> + if ((script_encoding_list = (zend_encoding **)compiler_globals->script_encoding_list)) {
>> + compiler_globals_ctor(compiler_globals, tsrm_ls);
>> + compiler_globals->script_encoding_list = (const zend_encoding **)script_encoding_list;
>> + } else {
>> + compiler_globals_ctor(compiler_globals, tsrm_ls);
>> + }
>> free(EG(zend_constants));
>> executor_globals_ctor(executor_globals, tsrm_ls);
>> global_persistent_list =&EG(persistent_list);
>>
>>
>> --
>> PHP CVS Mailing List (http://www.php.net/)
>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>
>
>


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Sat, May 5, 2012 at 2:36 PM, Dmitry Stogov <[email protected]> wrote:
> Hi Laruence,
>
> Thank you for sending this.
>
> I'm not sure if the patch is completely correct.
> With the patch all the threads share the single copy of script_encoding_list
> and when one thread terminates it calls compiler_globals_dtor() and frees
> the script_encoding_list. But other threads still keep reference to it.
>
> I think we have to duplicate script_encoding_list for each thread in the
> same way as we do for CG(function_table).
right, thanks
>
> Also I noticed a related issue. At zend.c compiler_globals_dtor()
> CG(script_encoding_list) deallocated using free() and in zend_multibyte.c
> zend_multibyte_set_script_encoding() using efree().
>
> I suppose the second place has to be fixed.
>
> I would appreciate if you could look into the problems.
okey, I will, thanks :)

>
> Thanks. Dmitry.
>
>
>
> On 05/03/2012 06:51 PM, Laruence wrote:
>>
>> Hi, Dmitry:
>>
>>      you may want to review this,  :)
>>
>> thanks
>> On Thu, May 3, 2012 at 10:39 PM, Xinchen Hui<[email protected]>  wrote:
>>>
>>> Commit:    72f19e9a8bcf5712b24fa333a26616eff19ac1ce
>>> Author:    Xinchen Hui<[email protected]>           Thu, 3 May 2012
>>> 22:39:53 +0800
>>> Parents:   d74d88fbb9c29b1dd5ff05a54b72cf7c9250955c
>>> Branches:  PHP-5.4
>>>
>>> Link:
>>> http://git.php.net/?p=php-src.git;a=commitdiff;h=72f19e9a8bcf5712b24fa333a26616eff19ac1ce
>>>
>>> Log:
>>> Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding config)
>>>
>>> Bugs:
>>> https://bugs.php.net/61922
>>>
>>> Changed paths:
>>>  M  NEWS
>>>  M  Zend/zend.c
>>>
>>>
>>> Diff:
>>> diff --git a/NEWS b/NEWS
>>> index 8796cf4..9ef6abf 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -10,6 +10,8 @@ PHP
>>>                    NEWS
>>>     (Laruence)
>>>
>>>  - Core:
>>> +  . Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding
>>> config).
>>> +    (Laruence)
>>>   . Fixed missing bound check in iptcparse(). (chris at chiappa.net)
>>>   . Fixed bug #61827 (incorrect \e processing on Windows) (Anatoliy)
>>>   . Fixed bug #61761 ('Overriding' a private static method with a
>>> different
>>> diff --git a/Zend/zend.c b/Zend/zend.c
>>> index dd299f1..37a1a27 100644
>>> --- a/Zend/zend.c
>>> +++ b/Zend/zend.c
>>> @@ -781,6 +781,8 @@ void zend_register_standard_ini_entries(TSRMLS_D) /*
>>> {{{ */
>>>  void zend_post_startup(TSRMLS_D) /* {{{ */
>>>  {
>>>  #ifdef ZTS
>>> +       zend_encoding **script_encoding_list;
>>> +
>>>        zend_compiler_globals *compiler_globals =
>>> ts_resource(compiler_globals_id);
>>>        zend_executor_globals *executor_globals =
>>> ts_resource(executor_globals_id);
>>>
>>> @@ -795,7 +797,12 @@ void zend_post_startup(TSRMLS_D) /* {{{ */
>>>        zend_destroy_rsrc_list(&EG(persistent_list) TSRMLS_CC);
>>>        free(compiler_globals->function_table);
>>>        free(compiler_globals->class_table);
>>> -       compiler_globals_ctor(compiler_globals, tsrm_ls);
>>> +       if ((script_encoding_list = (zend_encoding
>>> **)compiler_globals->script_encoding_list)) {
>>> +               compiler_globals_ctor(compiler_globals, tsrm_ls);
>>> +               compiler_globals->script_encoding_list = (const
>>> zend_encoding **)script_encoding_list;
>>> +       } else {
>>> +               compiler_globals_ctor(compiler_globals, tsrm_ls);
>>> +       }
>>>        free(EG(zend_constants));
>>>        executor_globals_ctor(executor_globals, tsrm_ls);
>>>        global_persistent_list =&EG(persistent_list);
>>>
>>>
>>> --
>>> PHP CVS Mailing List (http://www.php.net/)
>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>
>>
>>
>>
>



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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hi Dmitry:

On Sat, May 5, 2012 at 6:38 PM, Laruence <[email protected]> wrote:
> On Sat, May 5, 2012 at 2:36 PM, Dmitry Stogov <[email protected]> wrote:
>> Hi Laruence,
>>
>> Thank you for sending this.
>>
>> I'm not sure if the patch is completely correct.
>> With the patch all the threads share the single copy of script_encoding_list
>> and when one thread terminates it calls compiler_globals_dtor() and frees
>> the script_encoding_list. But other threads still keep reference to it.
>>
>> I think we have to duplicate script_encoding_list for each thread in the
>> same way as we do for CG(function_table).

after a further exam, this is right, there is a mechanism for new
thread re-configure inis(zend_ini_refresh_caches). then new thread
will have a copy.

> right, thanks
>>
>> Also I noticed a related issue. At zend.c compiler_globals_dtor()
>> CG(script_encoding_list) deallocated using free() and in zend_multibyte.c
>> zend_multibyte_set_script_encoding() using efree().
>>
>> I suppose the second place has to be fixed.
>>
>> I would appreciate if you could look into the problems.
and this should use free, I will fix it . however for now it's dead
codes, so no bug feedback. :)

thanks
> okey, I will, thanks :)
>
>>
>> Thanks. Dmitry.
>>
>>
>>
>> On 05/03/2012 06:51 PM, Laruence wrote:
>>>
>>> Hi, Dmitry:
>>>
>>>      you may want to review this,  :)
>>>
>>> thanks
>>> On Thu, May 3, 2012 at 10:39 PM, Xinchen Hui<[email protected]>  wrote:
>>>>
>>>> Commit:    72f19e9a8bcf5712b24fa333a26616eff19ac1ce
>>>> Author:    Xinchen Hui<[email protected]>           Thu, 3 May 2012
>>>> 22:39:53 +0800
>>>> Parents:   d74d88fbb9c29b1dd5ff05a54b72cf7c9250955c
>>>> Branches:  PHP-5.4
>>>>
>>>> Link:
>>>> http://git.php.net/?p=php-src.git;a=commitdiff;h=72f19e9a8bcf5712b24fa333a26616eff19ac1ce
>>>>
>>>> Log:
>>>> Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding config)
>>>>
>>>> Bugs:
>>>> https://bugs.php.net/61922
>>>>
>>>> Changed paths:
>>>>  M  NEWS
>>>>  M  Zend/zend.c
>>>>
>>>>
>>>> Diff:
>>>> diff --git a/NEWS b/NEWS
>>>> index 8796cf4..9ef6abf 100644
>>>> --- a/NEWS
>>>> +++ b/NEWS
>>>> @@ -10,6 +10,8 @@ PHP
>>>>                    NEWS
>>>>     (Laruence)
>>>>
>>>>  - Core:
>>>> +  . Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding
>>>> config).
>>>> +    (Laruence)
>>>>   . Fixed missing bound check in iptcparse(). (chris at chiappa.net)
>>>>   . Fixed bug #61827 (incorrect \e processing on Windows) (Anatoliy)
>>>>   . Fixed bug #61761 ('Overriding' a private static method with a
>>>> different
>>>> diff --git a/Zend/zend.c b/Zend/zend.c
>>>> index dd299f1..37a1a27 100644
>>>> --- a/Zend/zend.c
>>>> +++ b/Zend/zend.c
>>>> @@ -781,6 +781,8 @@ void zend_register_standard_ini_entries(TSRMLS_D) /*
>>>> {{{ */
>>>>  void zend_post_startup(TSRMLS_D) /* {{{ */
>>>>  {
>>>>  #ifdef ZTS
>>>> +       zend_encoding **script_encoding_list;
>>>> +
>>>>        zend_compiler_globals *compiler_globals =
>>>> ts_resource(compiler_globals_id);
>>>>        zend_executor_globals *executor_globals =
>>>> ts_resource(executor_globals_id);
>>>>
>>>> @@ -795,7 +797,12 @@ void zend_post_startup(TSRMLS_D) /* {{{ */
>>>>        zend_destroy_rsrc_list(&EG(persistent_list) TSRMLS_CC);
>>>>        free(compiler_globals->function_table);
>>>>        free(compiler_globals->class_table);
>>>> -       compiler_globals_ctor(compiler_globals, tsrm_ls);
>>>> +       if ((script_encoding_list = (zend_encoding
>>>> **)compiler_globals->script_encoding_list)) {
>>>> +               compiler_globals_ctor(compiler_globals, tsrm_ls);
>>>> +               compiler_globals->script_encoding_list = (const
>>>> zend_encoding **)script_encoding_list;
>>>> +       } else {
>>>> +               compiler_globals_ctor(compiler_globals, tsrm_ls);
>>>> +       }
>>>>        free(EG(zend_constants));
>>>>        executor_globals_ctor(executor_globals, tsrm_ls);
>>>>        global_persistent_list =&EG(persistent_list);
>>>>
>>>>
>>>> --
>>>> PHP CVS Mailing List (http://www.php.net/)
>>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>>
>>>
>>>
>>>
>>
>
>
>
> --
> Laruence  Xinchen Hui
> http://www.laruence.com/



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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 05/05/2012 06:57 PM, Laruence wrote:
> Hi Dmitry:
>
> On Sat, May 5, 2012 at 6:38 PM, Laruence<[email protected]> wrote:
>> On Sat, May 5, 2012 at 2:36 PM, Dmitry Stogov<[email protected]> wrote:
>>> Hi Laruence,
>>>
>>> Thank you for sending this.
>>>
>>> I'm not sure if the patch is completely correct.
>>> With the patch all the threads share the single copy of script_encoding_list
>>> and when one thread terminates it calls compiler_globals_dtor() and frees
>>> the script_encoding_list. But other threads still keep reference to it.
>>>
>>> I think we have to duplicate script_encoding_list for each thread in the
>>> same way as we do for CG(function_table).
>
> after a further exam, this is right, there is a mechanism for new
> thread re-configure inis(zend_ini_refresh_caches). then new thread
> will have a copy.

ok. In case you tested it, then your patch is fine.

Thanks. Dmitry.

>> right, thanks
>>>
>>> Also I noticed a related issue. At zend.c compiler_globals_dtor()
>>> CG(script_encoding_list) deallocated using free() and in zend_multibyte.c
>>> zend_multibyte_set_script_encoding() using efree().
>>>
>>> I suppose the second place has to be fixed.
>>>
>>> I would appreciate if you could look into the problems.
> and this should use free, I will fix it . however for now it's dead
> codes, so no bug feedback. :)
>
> thanks
>> okey, I will, thanks :)
>>
>>>
>>> Thanks. Dmitry.
>>>
>>>
>>>
>>> On 05/03/2012 06:51 PM, Laruence wrote:
>>>>
>>>> Hi, Dmitry:
>>>>
>>>> you may want to review this, :)
>>>>
>>>> thanks
>>>> On Thu, May 3, 2012 at 10:39 PM, Xinchen Hui<[email protected]> wrote:
>>>>>
>>>>> Commit: 72f19e9a8bcf5712b24fa333a26616eff19ac1ce
>>>>> Author: Xinchen Hui<[email protected]> Thu, 3 May 2012
>>>>> 22:39:53 +0800
>>>>> Parents: d74d88fbb9c29b1dd5ff05a54b72cf7c9250955c
>>>>> Branches: PHP-5.4
>>>>>
>>>>> Link:
>>>>> http://git.php.net/?p=php-src.git;a=commitdiff;h=72f19e9a8bcf5712b24fa333a26616eff19ac1ce
>>>>>
>>>>> Log:
>>>>> Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding config)
>>>>>
>>>>> Bugs:
>>>>> https://bugs.php.net/61922
>>>>>
>>>>> Changed paths:
>>>>> M NEWS
>>>>> M Zend/zend.c
>>>>>
>>>>>
>>>>> Diff:
>>>>> diff --git a/NEWS b/NEWS
>>>>> index 8796cf4..9ef6abf 100644
>>>>> --- a/NEWS
>>>>> +++ b/NEWS
>>>>> @@ -10,6 +10,8 @@ PHP
>>>>> NEWS
>>>>> (Laruence)
>>>>>
>>>>> - Core:
>>>>> + . Fixed bug #61922 (ZTS build doesn't accept zend.script_encoding
>>>>> config).
>>>>> + (Laruence)
>>>>> . Fixed missing bound check in iptcparse(). (chris at chiappa.net)
>>>>> . Fixed bug #61827 (incorrect \e processing on Windows) (Anatoliy)
>>>>> . Fixed bug #61761 ('Overriding' a private static method with a
>>>>> different
>>>>> diff --git a/Zend/zend.c b/Zend/zend.c
>>>>> index dd299f1..37a1a27 100644
>>>>> --- a/Zend/zend.c
>>>>> +++ b/Zend/zend.c
>>>>> @@ -781,6 +781,8 @@ void zend_register_standard_ini_entries(TSRMLS_D) /*
>>>>> {{{ */
>>>>> void zend_post_startup(TSRMLS_D) /* {{{ */
>>>>> {
>>>>> #ifdef ZTS
>>>>> + zend_encoding **script_encoding_list;
>>>>> +
>>>>> zend_compiler_globals *compiler_globals =
>>>>> ts_resource(compiler_globals_id);
>>>>> zend_executor_globals *executor_globals =
>>>>> ts_resource(executor_globals_id);
>>>>>
>>>>> @@ -795,7 +797,12 @@ void zend_post_startup(TSRMLS_D) /* {{{ */
>>>>> zend_destroy_rsrc_list(&EG(persistent_list) TSRMLS_CC);
>>>>> free(compiler_globals->function_table);
>>>>> free(compiler_globals->class_table);
>>>>> - compiler_globals_ctor(compiler_globals, tsrm_ls);
>>>>> + if ((script_encoding_list = (zend_encoding
>>>>> **)compiler_globals->script_encoding_list)) {
>>>>> + compiler_globals_ctor(compiler_globals, tsrm_ls);
>>>>> + compiler_globals->script_encoding_list = (const
>>>>> zend_encoding **)script_encoding_list;
>>>>> + } else {
>>>>> + compiler_globals_ctor(compiler_globals, tsrm_ls);
>>>>> + }
>>>>> free(EG(zend_constants));
>>>>> executor_globals_ctor(executor_globals, tsrm_ls);
>>>>> global_persistent_list =&EG(persistent_list);
>>>>>
>>>>>
>>>>> --
>>>>> PHP CVS Mailing List (http://www.php.net/)
>>>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>> --
>> Laruence Xinchen Hui
>> http://www.laruence.com/
>
>
>


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login