WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#29957 closed defect (bug) (fixed)

Hardcoded value of $chunk_size variable in class-IXR.php in wp-includes

Reported by: ankit.gade@… Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.4
Component: XML-RPC Keywords: has-patch
Focuses: performance Cc:

Description

Variable $chunk_size is used to restrict RAM usage to parse xml-rpc response message if the message is very large.
This variable value has been hard coded to 262144 (256 Kb). A filter should be provided for setting value of this variable.

It has following advantages.

  1. If message is large around 4-5 Mb, then getResponse() method of IXR_Client returns null, we can avoid this situation by setting $chunk_size to higher value.
  1. If server configuration is well versed, for example: If we have 32 GB RAM on server then allotting 5-8 Mb to $chunk_size should not be an issue and for the same filter can be used.

Attachments (3)

class-IXR.diff (913 bytes) - added by ankit.gade@… 5 years ago.
Filter added for setting value of $chunk_size
class-IXR.2.diff (990 bytes) - added by ankit.gade@… 5 years ago.
function_exists() check before applying filter.
class-IXR-revision-3.diff (970 bytes) - added by ankit.gade@… 4 years ago.
Changed filter name from xmlrpc_memory_limit to xmlrpc_chunk_parsing_size

Download all attachments as: .zip

Change History (16)

@ankit.gade@…
5 years ago

Filter added for setting value of $chunk_size

#1 @ankit.gade@…
5 years ago

  • Keywords has-patch added
  • Severity changed from normal to major

#2 follow-up: @georgestephanis
5 years ago

If a filter is added, it would need to have function_exists() conditionals wrapped around it.

An example of how that's been done in the one other place that it's happened can be seen here:

https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/class-IXR.php#L230

@ankit.gade@…
5 years ago

function_exists() check before applying filter.

#3 in reply to: ↑ 2 @ankit.gade@…
5 years ago

Thank you. New diff file has been added.

Replying to georgestephanis:

If a filter is added, it would need to have function_exists() conditionals wrapped around it.

An example of how that's been done in the one other place that it's happened can be seen here:

https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/class-IXR.php#L230

#4 @ankit.gade@…
5 years ago

  • Focuses performance added
  • Severity changed from major to critical

Will anybody notice this bug ? It's an useful thing to do because it creates critical issues during XML-RPC response parsing due to harcoded value.

#5 @georgestephanis
5 years ago

  • Severity changed from critical to normal

Please don't set the priority to critical, that's normally reserved for much more 'things are boing boom' issues. I've yet to see an example of things actually breaking due to this hardcoded chunk size.

Also, you just submitted this yesterday. Relax! The soonest it would get added in is potentially 4.1, which won't have a freeze for over a month yet.

There will be a trac scrub on Friday, as noted here - https://make.wordpress.org/core/2014/10/14/weekly-bug-scrubs-are-back/ - I'd suggest popping in to IRC then to speak to folks about this in #wordpress-dev.

#6 in reply to: ↑ description ; follow-up: @westi
5 years ago

  • Keywords reporter-feedback added

Replying to ankit.gade@…:

Variable $chunk_size is used to restrict RAM usage to parse xml-rpc response message if the message is very large.
This variable value has been hard coded to 262144 (256 Kb). A filter should be provided for setting value of this variable.

It has following advantages.

  1. If message is large around 4-5 Mb, then getResponse() method of IXR_Client returns null, we can avoid this situation by setting $chunk_size to higher value.
  1. If server configuration is well versed, for example: If we have 32 GB RAM on server then allotting 5-8 Mb to $chunk_size should not be an issue and for the same filter can be used.

Hey, Ankit Gade can you explain these bugs in more detail with some examples.

I'm not sure how change the chunk_size should affect the maximum response size that can be parsed.

It would be good to have some test date / a unit test which shows how this bug can be reproduced.

#7 in reply to: ↑ 6 @ankit.gade@…
5 years ago

  • Keywords reporter-feedback removed

Replying to westi:

Replying to ankit.gade@…:

Variable $chunk_size is used to restrict RAM usage to parse xml-rpc response message if the message is very large.
This variable value has been hard coded to 262144 (256 Kb). A filter should be provided for setting value of this variable.

It has following advantages.

  1. If message is large around 4-5 Mb, then getResponse() method of IXR_Client returns null, we can avoid this situation by setting $chunk_size to higher value.
  1. If server configuration is well versed, for example: If we have 32 GB RAM on server then allotting 5-8 Mb to $chunk_size should not be an issue and for the same filter can be used.

Hey, Ankit Gade can you explain these bugs in more detail with some examples.

I'm not sure how change the chunk_size should affect the maximum response size that can be parsed.

It would be good to have some test date / a unit test which shows how this bug can be reproduced.

Hello Peter,

Let me give you the scenario on which I am working currently and having such issue. Assume that we have two sites A and B, and I am currently working on site A.

  1. I am getting list of users present on site B using xml-rpc, the response message which I am getting is contains array of objects of users present on site B and the response message size is around 3 Mb. When I want to get that response in a variable using getresponse method of IXR_Client class, I am getting null. ( FYI, I have large value set for memory_limit in php.ini )
  1. Moreover, we have this piece of code in parse method of class IXR_Message
            do {
              if (strlen($this->message) <= $chunk_size) {
                  $final = true;
              }
              $part = substr($this->message, 0, $chunk_size);
              $this->message = substr($this->message, $chunk_size);
              if (!xml_parse($this->_parser, $part, $final)) {
                  return false;
              }
              if ($final) {
                  break;
              }
          } while (true);
    

If we have somewhat bigger $chunk_size set by filter ( In case we have good server resources ) then we can save the time we are iterating in do-while loop, since each time it will call functions it needs to to put arguments on stack etc.

So, in this case we can parse the message in single iteration.

Let me know if I am clear and in case you have any further query.

Thanks

#8 @wonderboymusic
4 years ago

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

In 35095:

XML-RPC: Add a filter, xmlrpc_memory_limit, to allow the value of $xmlrpc_memory_limit to be increased.

Props ankit.gade.
Fixes #29957.

#9 @wonderboymusic
4 years ago

  • Milestone changed from Awaiting Review to 4.4

#10 follow-up: @dd32
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This change feels confusing referencing memory in the way that it does, it's also not really a memory limit as the code now reflects.

I'd probably suggest something odd and vague would be a better option here, xmlrpc_chunk_parsing_size or similar.

@ankit.gade@…
4 years ago

Changed filter name from xmlrpc_memory_limit to xmlrpc_chunk_parsing_size

#11 in reply to: ↑ 10 @ankit.gade@…
4 years ago

Replying to dd32:

This change feels confusing referencing memory in the way that it does, it's also not really a memory limit as the code now reflects.

I'd probably suggest something odd and vague would be a better option here, xmlrpc_chunk_parsing_size or similar.

Word Memory has been used because $chunk_size is basically used to deal with efficient RAM usage, but I also agree with you that it may lead to confusion.

I have changed the name of filter and added revised patch.

#12 @wonderboymusic
4 years ago

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

In 35279:

XML-RPC: after [35095], change the name of the xmlrpc_memory_limit filter to xmlrpc_chunk_parsing_size.

Props ankit.gade.
Fixes #29957.

#13 @dd32
4 years ago

In 35364:

Update variable naming after [35279].

See #29957.

Note: See TracTickets for help on using tickets.