Make WordPress Core

Opened 5 years ago

Closed 3 weeks ago

Last modified 3 weeks ago

#49291 closed defect (bug) (fixed)

Code issues with set_time_limit

Reported by: madpeter's profile madpeter Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.8 Priority: normal
Severity: normal Version: 5.3.2
Component: Revisions Keywords: needs-patch good-first-bug
Focuses: administration, performance Cc:

Description (last modified by SergeyBiryukov)

file: wp-admin\includes\ajax-actions.php
function: wp_ajax_get_revision_diffs
line: 3425
Issue: no time limit set
Suggestion: set value to 120

file: wp-includes\class-pop3.php
function: __construct
line: 63
Issue: timeout value is not tested and can be set to 9999999 or a negitive value that acts the same as 0
Suggestion: Force a range for the value (1 to 120)

Attachments (1)

49291.diff (1.0 KB) - added by lovesoni1999 3 weeks ago.
Code issues with set_time_limit fix

Download all attachments as: .zip

Change History (12)

#1 @SergeyBiryukov
5 years ago

  • Component changed from Administration to Revisions
  • Description modified (diff)
  • Focuses administration added
  • Severity changed from major to normal

Hi there, welcome to WordPress Trac! Thanks for the report.

  • The set_time_limit( 0 ) instance in wp_ajax_get_revision_diffs() was introduced in [24707] / #24757. Could you clarify why the value should be changed to 120?
  • Please note that class-pop3.php is a part of the PHPMailer external library, any changes to its files should be submitted upstream: https://github.com/PHPMailer/PHPMailer.

#2 @madpeter
5 years ago

  • Keywords reporter-feedback added

The set_time_limit( 0 ) instance in wp_ajax_get_revision_diffs() was introduced in [24707] / #24757. Could you clarify why the value should be changed to 120?

if a badly coded plugin calls wp_ajax_get_revision_diffs but does not call set_time_limit itself and then it hangs due to a bug it can hold php active and not free up memory or worse use a while loop to hog cpu time.

but having a known limit for all set_time_limit calls it can help highlight issues with poorly coded plugins as it will create php time out warnings.

class-pop3.php
I will need to open a new ticket as wordpresses copy of phpmailer to far out of date.

#3 @madpeter
5 years ago

  • Keywords reporter-feedback removed

#4 @pbearne
2 months ago

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [24707]

Closing ticket

#5 @desrosj
3 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for another look.

@pbearne was [24707] the changeset you meant to share? That commit was 11 years ago but this ticket was only opened 5 years ago.

#6 @pbearne
3 weeks ago

Maybe not

Lets see if we can sort this

Paul

#7 @SergeyBiryukov
3 weeks ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 6.8

This appears to be the only instance of set_time_limit( 0 ) in core, i.e. removing the timeout limit completely.

Other instances have a reasonable limit instead, e.g. set_time_limit( 5 * MINUTE_IN_SECONDS ).

Looking at #24757 again, I think we could do the same here to avoid hanging processes.

@lovesoni1999
3 weeks ago

Code issues with set_time_limit fix

This ticket was mentioned in PR #7638 on WordPress/wordpress-develop by @lovesoni.


3 weeks ago
#8

  • Keywords has-patch added; needs-patch removed

Code issues with set_time_limit for ticket: 49291

Trac ticket: https://core.trac.wordpress.org/ticket/49291

I have set limit for timeout to 120 for smooth workflow of ajax and POP3 email

#9 @lovesoni1999
3 weeks ago

  • Keywords needs-patch added; has-patch removed

#10 @SergeyBiryukov
3 weeks ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from reopened to closed

In 59288:

Revisions: Set PHP time limit when generating revision diffs vis Ajax.

This aims to avoid hanging PHP processes if a plugin inadvertently attempts to request a large number of revision diffs.

Follow-up to [24520], [24707], [59039].

Props madpeter, lovesoni1999, debarghyabanerjee, pbearne, desrosj, SergeyBiryukov.
Fixes #49291.

@SergeyBiryukov commented on PR #7638:


3 weeks ago
#11

Thanks for the PR! Merged in r59288.

Note: See TracTickets for help on using tickets.