When receiving long responses from the server (my case 149 kB) preg_replace chokes in replacing the xml header.

Replacing this with a strpos() and substr() replacement I get my data parsed.

patch attached.

CommentFileSizeAuthor
#22 xmlrpc.patch1.3 KBmr.baileys
#20 xmlrpc.patch3.27 KBmr.baileys
#18 xmlrpc.patch1.53 KBmr.baileys
#15 265973-xmlrpc-parse-d7.patch1.17 KBdixon_
#15 265973-xmlrpc-parse-d6.patch1.18 KBdixon_
#13 265973-xmlrpc-parse.patch1.24 KBdamien tournoud
#6 265973-xmlrpc-parse.patch1.1 KBdamien tournoud
xmlrpc-choke.patch643 bytesclemens.tolboom

Comments

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

With chokes I mean it replaces the WHOLE message with an empty string. (Or was my data corrupt? afaik it was not)

My strpos searches for the first occurrence of '?>' while the preg_replace did an eager search for the last fitting '?>'

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

Steps to reproduce.
1. With devel php block insert the following code

xmlrpc();                                // lazy mans include
$message = str_repeat( '<a>a</a>', 128); // 1K string
$message = str_repeat( $message, 80);    // make n K in size
$message = '<e>'. $message .'</e>';      // wrap to make valid xml
$xml_message = xmlrpc_message( '<?xml version="1.0"?>'. $message); // create rpc message
xmlrpc_message_parse( $xml_message);     // and parse
var_dump( $xml_message);                 // check for message value

2. Run the php
3. Note that message part is around 87K in size
4. Now change the 80 into 128
5. See the var_dump message containing a NULL value.

Is this memory dependant?

gerhard killesreiter’s picture

gerhard killesreiter commented
Version: 5.7 » 7.x-dev

needs to go into D7 first.

gerhard killesreiter’s picture

gerhard killesreiter commented
Status: Active » Needs work

Does apply to D5 and D6 but needs work for D7.

damien tournoud’s picture

damien tournoud commented

This solution looks a little crude to me. What if there is a "?>" in the message but no XML header?

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review
StatusFileSize
new 265973-xmlrpc-parse.patch1.1 KB

Thinking about it, do we really need to strip the <?xml ?> tag.

I also noticed that the parser don't handle correctly the case when $xmlrpc_message->messagetype is not set at the end of the parsing (like in the case of a malformed message like the one above).

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

I wrote a test for this in #307477: xmlrpc test for big messages..

To run short I cannot reproduce with memory setting 64M.
Lowering to 32M same.
Lowering to 16M makes the whole testsuite die.

So I guess my test is wrong :(

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

I rewrote the test. And it runs ok.

So my guess is this bug is a 'cannot reproduce' unless my test is wrong.

Patch #6 should be investigated. Although I prefer a test for it first.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

On third thought ... this is probably PHP version dependent.

My previous PHP version was a Ubuntu 7.04 version on which I reported this issue. I'm not sure how to get this version number :-( On http://packages.ubuntu.com/ choosing Feisty ginve PHP version 5.2.1

My current PHP version is PHP 5.2.4-2ubuntu5.3

So if someone has this older version of PHP he can run the new test #307477: xmlrpc test for big messages..

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

The last submitted patch failed testing.

uccio’s picture

uccio commented
Component: other » base system
Status: Needs work » Patch (to be ported)

In my case the xmlrpc message is about 500K the php is 5.2.9 and the problem is preg_replace().
I have fix the problem in this way.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Component: base system » xml-rpc system

Moving to new 'xml-rpc system' component.

damien tournoud’s picture

damien tournoud commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 265973-xmlrpc-parse.patch1.24 KB

#529608: xmlrpc_message_parse() in xmlrpc.inc is not safe for big XML-RPC responses marked as a duplicate.

Here is a reroll of my patch from #6.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented

#13: 265973-xmlrpc-parse.patch queued for re-testing.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented
StatusFileSize
new 265973-xmlrpc-parse-d6.patch1.18 KB
new 265973-xmlrpc-parse-d7.patch1.17 KB

Here is a clean re-roll for both D7 and D6. I'd love to see this bug getting fixed as this is a real limitation when trying to post large stuff to Drupal via XMLRPC.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented
Priority: Normal » Critical

Setting this to critical as it is a broken behavior.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ includes/xmlrpc.inc	18 Feb 2010 13:38:04 -0000
@@ -161,8 +161,6 @@ function xmlrpc_message($message) {
-  // First remove the XML declaration
-  $xmlrpc_message->message = preg_replace('/<\?xml(.*)?\?' . '>/', '', $xmlrpc_message->message);
   if (trim($xmlrpc_message->message) == '') {
     return FALSE;
   }

If the response contains the XML declaration, then the directly following trim() and '' check will never be true?

+++ includes/xmlrpc.inc	18 Feb 2010 13:38:04 -0000
@@ -179,7 +177,10 @@ function xmlrpc_message_parse($xmlrpc_me
+  else if ($xmlrpc_message->messagetype == 'fault') {

There should be no space between "else" and "if".

Powered by Dreditor.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs work » Needs review
StatusFileSize
new xmlrpc.patch1.53 KB

The trim() and '' check is no longer required, as the addition of the $xmlrpc_message->messagetype check takes care of messages with an empty body. Dropped it and took care of the space in the else if.

As per Priority levels of Issues, I'm not sure this is critical.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Critical » Normal

Agreed on priority. Would be good to add a test that verifies we can parse either way correctly now.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
StatusFileSize
new xmlrpc.patch3.27 KB

The existing XML-RPC tests should be sufficient to assert parsing behavior for valid & well-formed messages. Added a number of unit tests to assert that invalid messages are rejected.

dries’s picture

dries commented
Version: 7.x-dev » 6.x-dev
Status: Needs review » Reviewed & tested by the community

Committed to CVS HEAD, but moving to Drupal 6. I think the patch in #15 is RTBC.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new xmlrpc.patch1.3 KB

Patch in #15 needed some tweaks (removal of the empty body check and elseif instead of else if).

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented
Status: Needs review » Reviewed & tested by the community

#22 applies just fine and receiving large posts also works now!

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Looked at CVS history, and this XML declaration removal seems to have been in there since the first moment of using our adapted incutio XML-RPC library. So since no reason was uncovered that we'd need it, committed the patch.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.