>> * I think a better strategy would be to just dup the file descriptor gotten
>> after the cast in curl_setopt, store it (instead of storing the zval) and
>> close it on curl handle destruction. This way we wouldn't have to worry
>> about zval refcounts or whether the file descriptor obtained is still valid.
>> Could you see if this other strategy works? (otherwise I can do it later
>> this week)
Ok, I made it to work (thanks for consultation to Pierre and
Johannes). You were right, the bug remains with curl_multi_exec too.
BTW it happens not only with CURL_STDERR but also with all other
options that take fp as a parameter (CURLOPT_FILE,
CURLOPT_WRITEHEADER, CURLOPT_INFILE) so I added them to the test and
made separate test for curl_multi_exec (see the patch).
After some thoughts I think this is the case when user really wants to
shoot into his leg - probably from user pov we should generate a
warning that we can't write into the supplied pointer (but actually we
can :)) like my previous patch did, but it raised a couple of
problems:
1) curl_multi_exec is called directly without interception from php
and create a wrapper just for this check seems like an overkill to me
2) we need to add 3 more checks to restore default values for all fp
options (see above) in two places: curl_exec and curl_multi_exec -
again overkill.
So I decided to go with this patch.
What do you think?
P.S. This patch is just againt trunk, if it's ok I will add 5.3 and
5.4 versions too.
--
Regards,
Shein Alexey
Index: trunk/ext/curl/php_curl.h
===================================================================
--- trunk/ext/curl/php_curl.h (revision 306938)
+++ trunk/ext/curl/php_curl.h (revision )
@@ -109,7 +109,7 @@
php_curl_write *write_header;
php_curl_read *read;
zval *passwd;
- zval *std_err;
+ FILE *std_err;
php_curl_progress *progress;
} php_curl_handlers;
Index: trunk/ext/curl/tests/bug48203.phpt
===================================================================
--- trunk/ext/curl/tests/bug48203.phpt (revision 281989)
+++ trunk/ext/curl/tests/bug48203.phpt (revision )
@@ -1,5 +1,5 @@
--TEST--
-Bug #48203 (Crash when CURLOPT_STDERR is set to regular file)
+Bug #48203 (Crash when file pointers passed to curl are closed before calling
curl_exec)
--SKIPIF--
<?php
if (!extension_loaded("curl")) {
@@ -12,22 +12,43 @@
--FILE--
<?php
+function checkForClosedFilePointer($curl_option, $description) {
-$fp = fopen(dirname(__FILE__) . '/bug48203.tmp', 'w');
+ $fp = fopen(dirname(__FILE__) . '/bug48203.tmp', 'w');
-$ch = curl_init();
+ $ch = curl_init();
+ // we also need CURLOPT_VERBOSE to be set to test CURLOPT_STDERR
properly
+ if (CURLOPT_STDERR == $curl_option) {
-curl_setopt($ch, CURLOPT_VERBOSE, 1);
+ curl_setopt($ch, CURLOPT_VERBOSE, 1);
-curl_setopt($ch, CURLOPT_STDERR, $fp);
-curl_setopt($ch, CURLOPT_URL, "");
+ }
+ curl_setopt($ch, $curl_option, $fp);
+
+ curl_setopt($ch, CURLOPT_URL, getenv("PHP_CURL_HTTP_REMOTE_SERVER"));
+ curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
+
-fclose($fp); // <-- premature close of $fp caused a crash!
+ fclose($fp); // <-- premature close of $fp caused a crash!
-curl_exec($ch);
+ curl_exec($ch);
-echo "Ok\n";
+ curl_close($ch);
+ echo "Ok for $description\n";
+}
+
+$options_to_check = array(
+ "CURLOPT_STDERR", "CURLOPT_WRITEHEADER", "CURLOPT_FILE",
"CURLOPT_INFILE"
+);
+
+foreach($options_to_check as $option) {
+ checkForClosedFilePointer(constant($option), $option);
+}
+
?>
--CLEAN--
<?php @unlink(dirname(__FILE__) . '/bug48203.tmp'); ?>
--EXPECT--
-Ok
+Ok for CURLOPT_STDERR
+Ok for CURLOPT_WRITEHEADER
+Ok for CURLOPT_FILE
+Ok for CURLOPT_INFILE
Index: trunk/ext/curl/interface.c
===================================================================
--- trunk/ext/curl/interface.c (revision 309881)
+++ trunk/ext/curl/interface.c (revision )
@@ -1810,7 +1810,10 @@
RETVAL_FALSE;
return 1;
}
-
+
+ // dup fp to be safe if user closes fp before calling
curl_exec
+ fp = fdopen(dup(fileno(fp)), ((php_stream *)
what)->mode);
+
error = CURLE_OK;
switch (option) {
case CURLOPT_FILE:
@@ -1845,11 +1848,7 @@
break;
case CURLOPT_STDERR:
if (((php_stream *) what)->mode[0] !=
'r' || ((php_stream *) what)->mode[1] == '+') {
- if (ch->handlers->std_err) {
-
zval_ptr_dtor(&ch->handlers->std_err);
- }
- zval_add_ref(zvalue);
- ch->handlers->std_err = *zvalue;
+ ch->handlers->std_err = fp;
} else {
php_error_docref(NULL
TSRMLS_CC, E_WARNING, "the provided file handle is not writable");
RETVAL_FALSE;
@@ -2520,11 +2519,6 @@
fprintf(stderr, "DTOR CALLED, ch = %x\n", ch);
#endif
- /* Prevent crash inside cURL if passed file has already been closed */
- if (ch->handlers->std_err && Z_REFCOUNT_P(ch->handlers->std_err) <= 0) {
- curl_easy_setopt(ch->cp, CURLOPT_STDERR, stderr);
- }
-
curl_easy_cleanup(ch->cp);
zend_llist_clean(&ch->to_free.str);
@@ -2559,9 +2553,6 @@
if (ch->handlers->passwd) {
zval_ptr_dtor(&ch->handlers->passwd);
}
- if (ch->handlers->std_err) {
- zval_ptr_dtor(&ch->handlers->std_err);
- }
if (ch->header.str_len > 0) {
efree(ch->header.str);
}
Index: trunk/ext/curl/tests/bug48203_multi.phpt
===================================================================
--- trunk/ext/curl/tests/bug48203_multi.phpt (revision )
+++ trunk/ext/curl/tests/bug48203_multi.phpt (revision )
@@ -0,0 +1,70 @@
+--TEST--
+Variation of bug #48203 with curl_multi_exec (Crash when file pointers passed
to curl are closed before calling curl_multi_exec)
+--SKIPIF--
+<?php
+if (!extension_loaded("curl")) {
+ exit("skip curl extension not loaded");
+}
+if (false === getenv('PHP_CURL_HTTP_REMOTE_SERVER')) {
+ exit("skip PHP_CURL_HTTP_REMOTE_SERVER env variable is not defined");
+}
+?>
+--FILE--
+<?php
+
+function checkForClosedFilePointer($curl_option, $description) {
+ $fp = fopen(dirname(__FILE__) . '/bug48203.tmp', 'w');
+
+ $ch1 = curl_init();
+ $ch2 = curl_init();
+
+ $options = array(
+ CURLOPT_RETURNTRANSFER => 1,
+ $curl_option => $fp,
+ CURLOPT_URL => getenv("PHP_CURL_HTTP_REMOTE_SERVER")
+ );
+
+ // we also need to set CURLOPT_VERBOSE to test CURLOPT_STDERR properly
+ if (CURLOPT_STDERR == $curl_option) {
+ $options[CURLOPT_VERBOSE] = 1;
+ }
+
+ curl_setopt_array($ch1, $options);
+ curl_setopt_array($ch2, $options);
+
+ fclose($fp); // <-- premature close of $fp caused a crash!
+
+ $mh = curl_multi_init();
+
+ curl_multi_add_handle($mh,$ch1);
+ curl_multi_add_handle($mh,$ch2);
+
+ $active = 0;
+ do {
+ curl_multi_exec($mh, $active);
+ } while ($active > 0);
+
+
+ curl_multi_remove_handle($mh, $ch1);
+ curl_multi_remove_handle($mh, $ch2);
+ curl_multi_close($mh);
+
+ echo "Ok for $description\n";
+}
+
+$options_to_check = array(
+ "CURLOPT_STDERR", "CURLOPT_WRITEHEADER", "CURLOPT_FILE",
"CURLOPT_INFILE"
+);
+
+foreach($options_to_check as $option) {
+ checkForClosedFilePointer(constant($option), $option);
+}
+
+?>
+--CLEAN--
+<?php @unlink(dirname(__FILE__) . '/bug48203.tmp'); ?>
+--EXPECT--
+Ok for CURLOPT_STDERR
+Ok for CURLOPT_WRITEHEADER
+Ok for CURLOPT_FILE
+Ok for CURLOPT_INFILE
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
