zedzedzed / table-of-contents-plus

https://wordpress.org/plugins/table-of-contents-plus/
GNU General Public License v2.0
53 stars 26 forks source link

Potential infinite loop in class-toc-widget.php #167

Open OldGrumpy-de opened 5 months ago

OldGrumpy-de commented 5 months ago

TOC_Widget::widget() calls do_shortcode() in line 39 [version 2402.1] which in combination with certain themes (I encountered the issue with DIVI 4.25.1) causes an infinite loop as DIVI proceeds to render the whole page including the sidebar when processing its shortcodes - which in turn calls TOC_Widget::widget() again until the PHP memory limit is reached and a hard OOM error is thrown.

While it is debatable who is the culprit, this issue is fixable with minimal effort in class-toc-widget.php by introducing a boolean property, for example $is_in_widget_processing, initializing it with false and checking its value right at the start of TOC_Widget::widget() - if it is already true, return right away without doing anything, otherwise set it to true, and only reset it to false at the two exit points of TOC_WIdget::widget().

Yes this looks a bit ugly but it seems to do the job quite well, tested it alot today (i.e. in the last 24 hours). Please let me know your opinion on this, I'd be glad if you could incorporate this or another solution into the plugin so I don't have to patch it manually each time an update is released. :)

zedzedzed commented 4 months ago

Thank you for the report.

I don't use Divi and do not include it in my testing so I can't speak to it at all. I know there have been a number of reports over the course of this plugin's life that there may be incompatibilities with the 2 but it was hard to invest my time further as I simply don't use Divi and there is no financial support stream to support it. I'm not sure if previous reports were similar to this, or could be wider - I really don't know.

I'm happy for others to step into that gap and provide PRs to support and maintain Divi.

For this particular case, I would suggest to check Divi's docs to see if there is an example or suggestions of how widgets are meant to be coded to avoid the looping case. It sounds like it could be a common problem for other widgets using Divi?

OldGrumpy-de commented 4 months ago

Thanks for your comment. In the meantime, I had more time to thoroughly look at the problem and it turned out it is partly a user error, too. But in the spirit of defensive programming, it would be great to have the plugin detect this and avoid a hard crash. Here is what happens:

a) User creates a new page in the visual builder. Main layout has no sidebar, but User wants this page to have one. b) User adds a sidebar block to the main page content instead of creating a template of the same layout and assign that to the new page. c) This turns the selected sidebar into an integral part of the main content, which causes havoc for every widget in the sidebar that uses the_content() or get_the_content() creating an infinite loop.

Side note: With the loop detector, the TOC will contain a section 0.x.y.z with the headings from the sidebar the TOC widget is placed in - technically fully correct, but a good indicator that something is wrong. Note: This only happens in the above situation, not generally.

I will report this scenario to the DIVI team too, because neither theme nor plugin are doing something wrong per se, it's just the proverbial corner case that "never happens" :)

Maybe it's a good idea to add a "doing it wrong" message in the loop detector so that others who run into this get a clearer idea of what's happening.

Considering the base reason, this is probably a situation that can be recreated with any theme that has a sidebar block placeable in the main content. Sadly I can't test this right away, but it looks like a plausible scenario to me.