- fix request errors hidden under the hood of the request pool
authorMichael Wallner <mike@php.net>
Fri, 28 Oct 2005 10:51:11 +0000 (10:51 +0000)
committerMichael Wallner <mike@php.net>
Fri, 28 Oct 2005 10:51:11 +0000 (10:51 +0000)
- smarter chunked decoder
- add exhaustive HttpRequestPool test
- add two tests for truncated chunked encoded data

http_encoding_api.c
http_request_pool_api.c
package.xml
package2.xml
tests/HttpRequestPool_003.phpt [new file with mode: 0644]
tests/chunked_decode_003.phpt [new file with mode: 0644]
tests/chunked_decode_004.phpt [new file with mode: 0644]

index 0062623fec3526d368d27cb61bdbbae41055369c..b4244f01131e610d2f3b9c0a97c387e1932f5b67 100644 (file)
 
 ZEND_EXTERN_MODULE_GLOBALS(http);
 
-static inline int eol_match(char **line, int *EOL_len)
+static inline int eol_match(char **line, int *eol_len)
 {
        char *ptr = *line;
        
        while (0x20 == *ptr) ++ptr;
 
-       if (ptr == http_locate_eol(*line, EOL_len)) {
+       if (ptr == http_locate_eol(*line, eol_len)) {
                *line = ptr;
                return 1;
        } else {
@@ -46,63 +46,69 @@ static inline int eol_match(char **line, int *EOL_len)
 /* {{{ char *http_encoding_dechunk(char *, size_t, char **, size_t *) */
 PHP_HTTP_API const char *_http_encoding_dechunk(const char *encoded, size_t encoded_len, char **decoded, size_t *decoded_len TSRMLS_DC)
 {
-       const char *e_ptr;
-       char *d_ptr;
-       long rest;
+       int eol_len = 0;
+       char *n_ptr = NULL;
+       const char *e_ptr = encoded;
        
        *decoded_len = 0;
        *decoded = ecalloc(1, encoded_len);
-       d_ptr = *decoded;
-       e_ptr = encoded;
-
-       while ((rest = encoded + encoded_len - e_ptr) > 0) {
-               long chunk_len = 0;
-               int EOL_len = 0, eol_mismatch = 0;
-               char *n_ptr;
-
-               chunk_len = strtol(e_ptr, &n_ptr, 16);
-
-               /* check if:
-                * - we could not read in chunk size
-                * - we got a negative chunk size
-                * - chunk size is greater then remaining size
-                * - chunk size is not followed by (CR)LF|NUL
-                */
-               if (    (n_ptr == e_ptr) ||     (chunk_len < 0) || (chunk_len > rest) || 
-                               (*n_ptr && (eol_mismatch = !eol_match(&n_ptr, &EOL_len)))) {
-                       /* don't fail on apperently not encoded data */
+
+       while ((encoded + encoded_len - e_ptr) > 0) {
+               unsigned long chunk_len = 0, rest;
+
+               chunk_len = strtoul(e_ptr, &n_ptr, 16);
+
+               /* we could not read in chunk size */
+               if (n_ptr == e_ptr) {
+                       /*
+                        * if this is the first turn and there doesn't seem to be a chunk
+                        * size at the begining of the body, do not fail on apparently
+                        * not encoded data and return a copy
+                        */
                        if (e_ptr == encoded) {
+                               http_error(HE_NOTICE, HTTP_E_ENCODING, "Data does not seem to be chunked encoded");
                                memcpy(*decoded, encoded, encoded_len);
                                *decoded_len = encoded_len;
                                return encoded + encoded_len;
                        } else {
                                efree(*decoded);
-                               if (eol_mismatch) {
-                                       if (EOL_len == 2) {
-                                               http_error_ex(HE_WARNING, HTTP_E_ENCODING, "Invalid character (expected 0x0D 0x0A; got: 0x%02X 0x%02X)", *n_ptr, *(n_ptr + 1));
-                                       } else {
-                                               http_error_ex(HE_WARNING, HTTP_E_ENCODING, "Invalid character (expected 0x0A; got: 0x%02X)", *n_ptr);
-                                       }
-                               } else {
-                                       char *error = estrndup(e_ptr, strcspn(n_ptr, "\r\n "));
-                                       http_error_ex(HE_WARNING, HTTP_E_ENCODING, "Invalid chunk size: '%s' at pos %ld of %lu", error, (long) (n_ptr - encoded), (unsigned long) encoded_len);
-                                       efree(error);
-                               }
+                               http_error_ex(HE_WARNING, HTTP_E_ENCODING, "Expected chunk size at pos %lu of %lu but got trash", (unsigned long) (n_ptr - encoded), (unsigned long) encoded_len);
                                return NULL;
                        }
-               } else {
-                       e_ptr = n_ptr;
                }
-
+               
                /* reached the end */
                if (!chunk_len) {
                        break;
                }
 
-               memcpy(d_ptr, e_ptr += EOL_len, chunk_len);
-               d_ptr += chunk_len;
-               e_ptr += chunk_len + EOL_len;
+               /* there should be CRLF after the chunk size, but we'll ignore SP+ too */
+               if (*n_ptr && !eol_match(&n_ptr, &eol_len)) {
+                       if (eol_len == 2) {
+                               http_error_ex(HE_WARNING, HTTP_E_ENCODING, "Expected CRLF at pos %lu of %lu but got 0x%02X 0x%02X", (unsigned long) (n_ptr - encoded), (unsigned long) encoded_len, *n_ptr, *(n_ptr + 1));
+                       } else {
+                               http_error_ex(HE_WARNING, HTTP_E_ENCODING, "Expected LF at pos %lu of %lu but got 0x%02X", (unsigned long) (n_ptr - encoded), (unsigned long) encoded_len, *n_ptr);
+                       }
+               }
+               n_ptr += eol_len;
+               
+               /* chunk size pretends more data than we actually got, so it's probably a truncated message */
+               if (chunk_len > (rest = encoded + encoded_len - n_ptr)) {
+                       http_error_ex(HE_WARNING, HTTP_E_ENCODING, "Truncated message: chunk size %lu exceeds remaining data size %lu at pos %lu of %lu", chunk_len, rest, (unsigned long) (n_ptr - encoded), (unsigned long) encoded_len);
+                       chunk_len = rest;
+               }
+
+               /* copy the chunk */
+               memcpy(*decoded + *decoded_len, n_ptr, chunk_len);
                *decoded_len += chunk_len;
+               
+               if (chunk_len == rest) {
+                       e_ptr = n_ptr + chunk_len;
+                       break;
+               } else {
+                       /* advance to next chunk */
+                       e_ptr = n_ptr + chunk_len + eol_len;
+               }
        }
 
        return e_ptr;
index ca7aec756ad41c40f480159896ce28a5157fb0f3..0faf2756c13f5fae199891bfd1eb94e73c9d60f6 100644 (file)
@@ -239,6 +239,12 @@ PHP_HTTP_API void _http_request_pool_dtor(http_request_pool *pool TSRMLS_DC)
 }
 /* }}} */
 
+#ifdef PHP_WIN32
+#      define SELECT_ERROR SOCKET_ERROR
+#else
+#      define SELECT_ERROR -1
+#endif
+
 /* {{{ STATUS http_request_pool_select(http_request_pool *) */
 PHP_HTTP_API STATUS _http_request_pool_select(http_request_pool *pool)
 {
@@ -250,12 +256,12 @@ PHP_HTTP_API STATUS _http_request_pool_select(http_request_pool *pool)
        FD_ZERO(&W);
        FD_ZERO(&E);
 
-       curl_multi_fdset(pool->ch, &R, &W, &E, &MAX);
-#ifdef PHP_WIN32
-       return (SOCKET_ERROR != select(MAX + 1, &R, &W, &E, &timeout)) ? SUCCESS : FAILURE;
-#else
-       return (-1 != select(MAX + 1, &R, &W, &E, &timeout)) ? SUCCESS : FAILURE;
-#endif
+       if (CURLM_OK == curl_multi_fdset(pool->ch, &R, &W, &E, &MAX)) {
+               if (MAX == -1 || SELECT_ERROR != select(MAX + 1, &R, &W, &E, &timeout)) {
+                       return SUCCESS;
+               }
+       }
+       return FAILURE;
 }
 /* }}} */
 
@@ -269,11 +275,9 @@ PHP_HTTP_API int _http_request_pool_perform(http_request_pool *pool TSRMLS_DC)
        
        while (msg = curl_multi_info_read(pool->ch, &remaining)) {
                if (CURLMSG_DONE == msg->msg) {
-
-#if HTTP_DEBUG_REQPOOLS
-                       fprintf(stderr, "Done CURL handle: %p (remaining: %d)\n", msg->easy_handle, remaining);
-#endif
-                       
+                       if (CURLE_OK != msg->data.result) {
+                               http_error(HE_WARNING, HTTP_E_REQUEST, curl_easy_strerror(msg->data.result));
+                       }
                        zend_llist_apply_with_argument(&pool->handles, (llist_apply_with_arg_func_t) http_request_pool_responsehandler, msg->easy_handle TSRMLS_CC);
                }
        }
index 74d222162ab99c24610b55c69e3a4d7d665bed89..4cda6aeec15eb879ddfba2db2d0b539e91583597 100644 (file)
   </maintainers>
  <release>
   <version>0.16.0</version>
-  <date>2005-10-24</date>
+  <date>2005-10-28</date>
   <license>BSD, revised</license>
   <state>beta</state>
   <notes>+ Added ext/zlib independant GZIP support
 + Added HttpRequestPool::getAttachedRequests() and getFinishedRequests()
-+ Added experimental thread safety for curl-gnutls builds
++ Added experimental thread safety for builds linking against libcurl-gnutls
++ Improved the chunked decoder
+
+- License change!
+
+* Fixed a bug where HttpRequest warnings were hidden within the HttpRequestPool
   </notes>
   <deps>
    <dep type="php" rel="ge" version="4.3"/>
     <file role="test" name="abs_uri_002.phpt"/>
     <file role="test" name="allowed_methods_001.phpt"/>
     <file role="test" name="allowed_methods_001_logging.phpt"/>
+    <file role="test" name="build_uri_001.phpt"/>
     <file role="test" name="chunked_decode_001.phpt"/>
     <file role="test" name="chunked_decode_002.phpt"/>
+    <file role="test" name="chunked_decode_003.phpt"/>
+    <file role="test" name="chunked_decode_004.phpt"/>
+    <file role="test" name="cloning_001.phpt"/>
     <file role="test" name="data.txt"/>
     <file role="test" name="date_001.phpt"/>
     <file role="test" name="date_002.phpt"/>
@@ -85,6 +94,7 @@
     <file role="test" name="HttpMessage_001.phpt"/>
     <file role="test" name="HttpRequestPool_001.phpt"/>
     <file role="test" name="HttpRequestPool_002.phpt"/>
+    <file role="test" name="HttpRequestPool_003.phpt"/>
     <file role="test" name="HttpRequest_001.phpt"/>
     <file role="test" name="HttpRequest_002.phpt"/>
     <file role="test" name="HttpRequest_003.phpt"/>
     <file role="test" name="INI_001.phpt"/>
     <file role="test" name="log.inc"/>
     <file role="test" name="parse_headers_001.phpt"/>
+    <file role="test" name="parse_message_001.phpt"/>
     <file role="test" name="parse_message_002.phpt"/>
     <file role="test" name="parse_message_003.phpt"/>
     <file role="test" name="parse_message_004.phpt"/>
     <file role="test" name="redirect_003.phpt"/>
     <file role="test" name="redirect_003_logging.phpt"/>
     <file role="test" name="request_gzip.phpt"/>
+    <file role="test" name="request_methods.phpt"/>
     <file role="test" name="send_data_001.phpt"/>
     <file role="test" name="send_data_002.phpt"/>
     <file role="test" name="send_data_003.phpt"/>
     <file role="test" name="send_file_006.phpt"/>
     <file role="test" name="send_file_007.phpt"/>
     <file role="test" name="skip.inc"/>
+    <file role="test" name="urls.txt"/>
    </dir> <!-- /tests -->
    <file role="src" name="config.m4"/>
    <file role="src" name="config.w32"/>
    <file role="src" name="http_url_api.c"/>
    <file role="src" name="http_util_object.c"/>
    <file role="doc" name="KnownIssues.txt"/>
+   <file role="doc" name="LICENSE"/>
    <file role="src" name="Makefile.frag"/>
    <file role="src" name="missing.c"/>
    <file role="src" name="missing.h"/>
index 2bab9b3133564733e66d1da306f7afcebd801fa5..bccc86e165eeb6f027143012c0a1d459eaf78d6b 100644 (file)
  <notes><![CDATA[
 + Added ext/zlib independant GZIP support
 + Added HttpRequestPool::getAttachedRequests() and getFinishedRequests()
-+ Added experimental thread safety for curl-gnutls builds
++ Added experimental thread safety for builds linking against libcurl-gnutls
++ Improved the chunked decoder
+
+- License change!
+
+* Fixed a bug where HttpRequest warnings were hidden within the HttpRequestPool
 ]]></notes>
  <contents>
   <dir name="/">
    
    <dir name="tests">
     <file role="test" name="data.txt"/>
+    <file role="test" name="urls.txt"/>
     <file role="test" name="skip.inc"/>
     <file role="test" name="log.inc"/>
     <file role="test" name="abs_uri_001.phpt"/>
     <file role="test" name="abs_uri_002.phpt"/>
     <file role="test" name="allowed_methods_001.phpt"/>
     <file role="test" name="allowed_methods_001_logging.phpt"/>
+    <file role="test" name="build_uri_001.phpt"/>
     <file role="test" name="chunked_decode_001.phpt"/>
     <file role="test" name="chunked_decode_002.phpt"/>
+    <file role="test" name="chunked_decode_003.phpt"/>
+    <file role="test" name="chunked_decode_004.phpt"/>
+    <file role="test" name="cloning_001.phpt"/>
     <file role="test" name="date_001.phpt"/>
     <file role="test" name="date_002.phpt"/>
     <file role="test" name="encodings.phpt"/>
     <file role="test" name="HttpMessage_001.phpt"/>
     <file role="test" name="HttpRequestPool_001.phpt"/>
     <file role="test" name="HttpRequestPool_002.phpt"/>
+    <file role="test" name="HttpRequestPool_003.phpt"/>
     <file role="test" name="HttpRequest_001.phpt"/>
     <file role="test" name="HttpRequest_002.phpt"/>
     <file role="test" name="HttpRequest_003.phpt"/>
     <file role="test" name="HttpResponse_004.phpt"/>
     <file role="test" name="INI_001.phpt"/>
     <file role="test" name="parse_headers_001.phpt"/>
+    <file role="test" name="parse_message_001.phpt"/>
     <file role="test" name="parse_message_002.phpt"/>
     <file role="test" name="parse_message_003.phpt"/>
     <file role="test" name="parse_message_004.phpt"/>
     <file role="test" name="redirect_003.phpt"/>
     <file role="test" name="redirect_003_logging.phpt"/>
     <file role="test" name="request_gzip.phpt"/>
+    <file role="test" name="request_methods.phpt"/>
     <file role="test" name="send_data_001.phpt"/>
     <file role="test" name="send_data_002.phpt"/>
     <file role="test" name="send_data_003.phpt"/>
diff --git a/tests/HttpRequestPool_003.phpt b/tests/HttpRequestPool_003.phpt
new file mode 100644 (file)
index 0000000..c89efa4
--- /dev/null
@@ -0,0 +1,171 @@
+--TEST--
+HttpRequestPool chain
+--SKIPIF--
+<?php
+inlcude 'skip.inc';
+checkcls('HttpRequest');
+checkcls('HttpRequestPool');
+?>
+--FILE--
+<?php
+
+echo "-TEST\n";
+
+set_time_limit(0);
+ini_set('error_reporting', E_ALL);
+
+class Pool extends HttpRequestPool
+{
+       private $all;
+       private $rem;
+       private $dir;
+       
+       public final function __construct($urls_file = 'urls.txt', $cache_dir = 'HttpRequestPool_cache')
+       {
+               $this->dir = (is_dir($cache_dir) or @mkdir($cache_dir)) ? $cache_dir : null;
+               
+               foreach (array_map('trim', file($urls_file)) as $url) {
+                       $this->all[$url] = $this->dir ? $this->dir .'/'. md5($url) : null;
+               }
+               
+               $this->send();
+       }
+       
+       public final function send()
+       {
+               if (RMAX) {
+                       $now = array_slice($this->all, 0, RMAX);
+                       $this->rem = array_slice($this->all, RMAX);
+               } else {
+                       $now = $urls;
+                       $this->rem = array();
+               }
+               
+               foreach ($now as $url => $file) {
+                       $this->attach(
+                               new HttpRequest(
+                                       $url,
+                                       HttpRequest::METH_GET,
+                                       array(
+                                               'redirect'      => 5,
+                                               'timeout'       => 50,
+                                               'connecttimeout' => 10,
+                                               'lastmodified' => is_file($file)?filemtime($file):0
+                                       )
+                               )
+                       );
+               }
+               
+               while ($this->socketPerform()) {
+                       if (!$this->socketSelect()) {
+                               throw new HttpSocketException;
+                       }
+               }
+       }
+       
+       protected final function socketPerform()
+       {
+               try {
+                       $rc = parent::socketPerform();
+               } catch (HttpRequestException $x) {
+                       // a request may have thrown an exception,
+                       // but it is still save to continue
+                       echo $x->getMessage(), "\n";
+               }
+               
+               foreach ($this->getFinishedRequests() as $r) {
+                       $this->detach($r);
+                       
+                       $u = $r->getUrl();
+                       $c = $r->getResponseCode();
+                       $b = $r->getResponseBody();
+                       
+                       printf("%d %s %d\n", $c, $u, strlen($b));
+                       
+                       if ($c == 200 && $this->dir) {
+                               file_put_contents($this->all[$u], $b);
+                       }
+                       
+                       if ($a = each($this->rem)) {
+                               list($url, $file) = $a;
+                               $this->attach(
+                                       new HttpRequest(
+                                               $url,
+                                               HttpRequest::METH_GET,
+                                               array(
+                                                       'redirect'      => 3,
+                                                       'timeout'       => 25,
+                                                       'compress'      => true,
+                                                       'connecttimeout' => 10,
+                                                       'lastmodified' => is_file($file)?filemtime($file):0
+                                               )
+                                       )
+                               );
+                       }
+               }
+               return $rc;
+       }
+}
+
+define('RMAX', 10);
+chdir(dirname(__FILE__));
+
+$time = microtime(true);
+new Pool();
+printf("Elapsed: %0.3fs\n", microtime(true)-$time);
+
+echo "Done\n";
+?>
+--EXPECTF--
+%sTEST
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+%d %s %d
+Elapsed: %fs
+Done
diff --git a/tests/chunked_decode_003.phpt b/tests/chunked_decode_003.phpt
new file mode 100644 (file)
index 0000000..5af29a4
--- /dev/null
@@ -0,0 +1,27 @@
+--TEST--
+http_chunked_decode() truncated message
+--SKIPIF--
+<?php
+include 'skip.inc';
+?>
+--FILE--
+<?php
+echo "-TEST\n";
+$data =
+"02\r\n".
+"ab\r\n".
+"04\r\n".
+"ra\nc\r\n".
+"06\r\n".
+"adabra\r\n".
+"ff\r\n".
+"\nall we got\n";
+var_dump(http_chunked_decode($data));
+?>
+--EXPECTF--
+%sTEST
+%sWarning%s:%shttp_chunked_decode()%sTruncated message: chunk size %d exceeds remaining data size %d at pos %d of %d%s
+string(24) "abra
+cadabra
+all we got
+"
diff --git a/tests/chunked_decode_004.phpt b/tests/chunked_decode_004.phpt
new file mode 100644 (file)
index 0000000..77aed28
--- /dev/null
@@ -0,0 +1,26 @@
+--TEST--
+http_chunked_decode() truncated message ending with NUL after a chunk
+--SKIPIF--
+<?php
+include 'skip.inc';
+?>
+--FILE--
+<?php
+echo "-TEST\n";
+$data =
+"02\r\n".
+"ab\r\n".
+"04\r\n".
+"ra\nc\r\n".
+"06\r\n".
+"adabra\r\n".
+"0c\r\n".
+"\nall we got\n";
+var_dump(http_chunked_decode($data));
+?>
+--EXPECTF--
+%sTEST
+string(24) "abra
+cadabra
+all we got
+"