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

Fix high opcache.interned_strings_buffer causing shm corruption #9260

Merged
merged 3 commits into from
Sep 3, 2022

Conversation

arnaud-lb
Copy link
Member

Fixes #9259

zend_long size = zend_ini_parse_quantity_warn(new_value, entry->name);

if (size < 0) {
zend_accel_error(ACCEL_LOG_WARNING, "opcache.interned_strings_buffer must be higher than or equal to 0, " ZEND_LONG_FMT " given.\n", size);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
zend_accel_error(ACCEL_LOG_WARNING, "opcache.interned_strings_buffer must be higher than or equal to 0, " ZEND_LONG_FMT " given.\n", size);
zend_accel_error(ACCEL_LOG_WARNING, "opcache.interned_strings_buffer must be greater than or equal to 0, " ZEND_LONG_FMT " given.\n", size);

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

@arnaud-lb arnaud-lb marked this pull request as ready for review August 6, 2022 12:09
zend_accel_error(ACCEL_LOG_WARNING, "opcache.interned_strings_buffer must be greater than or equal to 0, " ZEND_LONG_FMT " given.\n", size);
return FAILURE;
}
if (size > MAX_INTERNED_STRINGS_BUFFER_SIZE) {
Copy link
Member

Choose a reason for hiding this comment

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

I do not know for certain if we should return an error for both cases, as you said in the ticket, it can me a mistake. However that fix the issue alright whatsoever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wanted to be consistent with OnUpdateMemoryConsumption, but maybe returning a FAILURE in both cases here would be better.

In my opinion, ini handler failures during startup/activate should also result in a fatal error, but that's for an other PR/RFC.

@krakjoe krakjoe requested a review from dstogov August 10, 2022 13:54
@arnaud-lb arnaud-lb changed the base branch from master to PHP-8.2 September 2, 2022 14:50
@arnaud-lb arnaud-lb merged commit fb242f4 into php:PHP-8.2 Sep 3, 2022
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Sep 3, 2022
* PHP-8.2:
  [ci skip] NEWS
  Log the cause of error when opcache cannot write to file cache (php#9258)
  Fix high opcache.interned_strings_buffer causing shm corruption (php#9260)
@iluuu1994
Copy link
Member

It looks like this test triggers memory leaks with ASAN.

https://github.com/php/php-src/runs/8281104519?check_suite_focus=true

@arnaud-lb Can you have a look? There's no hurry.

@iluuu1994
Copy link
Member

I marked the tests as xfail when running with ASAN for now.

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

Successfully merging this pull request may close these issues.

Setting opcache.interned_strings_buffer to a very high value leads to corruption of shm
5 participants