It seems my last letter didn't came to the list, resending it. So this is new version of this patch - with curl_multi_exec involved and accordingly changed tests. Let me know what you think about it.
Additionally, I found another test case (actually two) not handled yet (see attached test case in second file): 1) If fp of non-existant file is setup to stderr and is closed before calling curl_close() it causes glibc memory error 2) if I additionally unlink file before curl_close() - it causes segfault (fie not found in io_write.c or smth like that) -- Regards, Shein Alexey -- 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 ) @@ -150,6 +150,7 @@ void _php_curl_cleanup_handle(php_curl *); void _php_curl_multi_cleanup_list(void *data); +int _php_curl_file_pointers_correct(php_curl *ch TSRMLS_DC); /* streams support */ Index: trunk/ext/curl/multi.c =================================================================== --- trunk/ext/curl/multi.c (revision 306938) +++ trunk/ext/curl/multi.c (revision ) @@ -130,6 +130,16 @@ } /* }}} */ +static long _php_curl_multi_remove_handle(php_curlm *mh, php_curl *ch) +{ + --ch->uses; + + zend_llist_del_element( &mh->easyh, &ch, + (int (*)(void *, void *)) curl_compare_resources ); + + return (long) curl_multi_remove_handle(mh->multi, ch->cp); +} + /* {{{ proto int curl_multi_remove_handle(resource mh, resource ch) Remove a multi handle from a set of cURL handles */ PHP_FUNCTION(curl_multi_remove_handle) @@ -146,12 +156,7 @@ ZEND_FETCH_RESOURCE(mh, php_curlm *, &z_mh, -1, le_curl_multi_handle_name, le_curl_multi_handle); ZEND_FETCH_RESOURCE(ch, php_curl *, &z_ch, -1, le_curl_name, le_curl); - --ch->uses; - - zend_llist_del_element( &mh->easyh, &z_ch, - (int (*)(void *, void *)) curl_compare_resources ); - - RETURN_LONG((long) curl_multi_remove_handle(mh->multi, ch->cp)); + RETURN_LONG(_php_curl_multi_remove_handle(mh, ch)); } /* }}} */ @@ -211,6 +216,22 @@ ZEND_FETCH_RESOURCE(mh, php_curlm *, &z_mh, -1, le_curl_multi_handle_name, le_curl_multi_handle); + { + zend_llist_position pos; + php_curl *ch; + zval *ch_zval; + + /* search the list of easy handles hanging off the multi-handle */ + for(ch_zval = (zval *)zend_llist_get_first_ex(&mh->easyh, &pos); ch_zval; + ch_zval = (zval *)zend_llist_get_next_ex(&mh->easyh, &pos)) { + ZEND_FETCH_RESOURCE(ch, php_curl *, &ch_zval, -1, le_curl_name, le_curl); + + if (!_php_curl_file_pointers_correct(ch TSRMLS_CC)) { + _php_curl_multi_remove_handle(mh, ch); + } + } + } + convert_to_long_ex(&z_still_running); still_running = Z_LVAL_P(z_still_running); result = curl_multi_perform(mh->multi, &still_running); 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,68 @@ --FILE-- <?php -$fp = fopen(dirname(__FILE__) . '/bug48203.tmp', 'w'); +function checkForClosedFilePointer($curl_option, $description) { + $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, ""); + } + if (CURLOPT_INFILE == $curl_option) { + curl_setopt($ch, CURLOPT_UPLOAD, 1); + } + + curl_setopt($ch, $curl_option, $fp); + + curl_setopt($ch, CURLOPT_URL, 'localhost'); + 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 +--EXPECTF-- +Warning: curl_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_exec(): CURLOPT_STDERR handle is incorrect in %s on line %d +* About to connect() %a +* Closing connection #%d +Ok for CURLOPT_STDERR + +Warning: curl_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_exec(): CURLOPT_WRITEHEADER handle is incorrect in %s on line %d +Ok for CURLOPT_WRITEHEADER + +Warning: curl_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_exec(): CURLOPT_FILE handle is incorrect in %s on line %d +%a +Ok for CURLOPT_FILE + +Warning: curl_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_exec(): CURLOPT_INFILE handle is incorrect in %s on line %d +Ok for CURLOPT_INFILE Index: trunk/ext/curl/interface.c =================================================================== --- trunk/ext/curl/interface.c (revision 309881) +++ trunk/ext/curl/interface.c (revision ) @@ -2190,6 +2190,41 @@ } /* }}} */ +int _php_curl_check_file_pointer(zval *zv_stream, const char* filehandle_type TSRMLS_DC) +{ + php_stream *stream; + if (zv_stream) { + if (!(php_stream_from_zval_no_verify(stream, &zv_stream))) { + php_error_docref(NULL TSRMLS_CC, E_WARNING, "%s handle is incorrect", filehandle_type); + return 0; + } + } + + return 1; +} + +int _php_curl_file_pointers_correct(php_curl *ch TSRMLS_DC) +{ + if (!_php_curl_check_file_pointer(ch->handlers->std_err, "CURLOPT_STDERR" TSRMLS_CC)) { + curl_easy_setopt(ch->cp, CURLOPT_STDERR, stderr); + } + + if (!_php_curl_check_file_pointer(ch->handlers->write->stream, "CURLOPT_FILE" TSRMLS_CC)) { + ch->handlers->write->method = PHP_CURL_STDOUT; + } + + if (!_php_curl_check_file_pointer(ch->handlers->write_header->stream, "CURLOPT_WRITEHEADER" TSRMLS_CC)) { + ch->handlers->write_header->method = PHP_CURL_IGNORE; + } + + // this is critical since there's no point in upload with corrupted file + if (!_php_curl_check_file_pointer(ch->handlers->read->stream, "CURLOPT_INFILE" TSRMLS_CC)) { + return 0; + } + + return 1; +} + /* {{{ proto bool curl_exec(resource ch) Perform a cURL session */ PHP_FUNCTION(curl_exec) @@ -2204,6 +2239,10 @@ ZEND_FETCH_RESOURCE(ch, php_curl *, &zid, -1, le_curl_name, le_curl); + if (!_php_curl_file_pointers_correct(ch TSRMLS_CC)) { + RETURN_FALSE; + } + _php_curl_cleanup_handle(ch); error = curl_easy_perform(ch->cp); 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,106 @@ +--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; + } + + if (CURLOPT_INFILE == $curl_option) { + $options[CURLOPT_UPLOAD] = 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'); ?> +--EXPECTF-- +Warning: curl_multi_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_multi_exec(): CURLOPT_STDERR handle is incorrect in %s on line %d + +Warning: curl_multi_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_multi_exec(): CURLOPT_STDERR handle is incorrect in %s on line %d +%a +Ok for CURLOPT_STDERR + +Warning: curl_multi_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_multi_exec(): CURLOPT_WRITEHEADER handle is incorrect in %s on line %d + +Warning: curl_multi_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_multi_exec(): CURLOPT_WRITEHEADER handle is incorrect in %s on line %d +%AOk for CURLOPT_WRITEHEADER + +Warning: curl_multi_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_multi_exec(): CURLOPT_FILE handle is incorrect in %s on line %d + +Warning: curl_multi_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_multi_exec(): CURLOPT_FILE handle is incorrect in %s on line %d +%a +Ok for CURLOPT_FILE + +Warning: curl_multi_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_multi_exec(): CURLOPT_INFILE handle is incorrect in %s on line %d + +Warning: curl_multi_exec(): %d is not a valid stream resource in %s on line %d + +Warning: curl_multi_exec(): CURLOPT_INFILE handle is incorrect in %s on line %d +Ok for CURLOPT_INFILE
Index: trunk/ext/curl/tests/curl_file_deleted_before_curl_close.phpt =================================================================== --- trunk/ext/curl/tests/curl_file_deleted_before_curl_close.phpt (revision ) +++ trunk/ext/curl/tests/curl_file_deleted_before_curl_close.phpt (revision ) @@ -0,0 +1,37 @@ +--TEST-- +Memory corruption error if fp of just created file is closed before curl_close. +--CREDITS-- +Alexey Shein <con...@gmail.com> +--SKIPIF-- +<?php if (!extension_loaded("curl") || false === getenv('PHP_CURL_HTTP_REMOTE_SERVER')) print "skip"; ?> +--FILE-- +<?php + +$ch = curl_init(getenv('PHP_CURL_HTTP_REMOTE_SERVER')); + +$temp_file = dirname(__FILE__) . '/curl_file_deleted_before_curl_close.tmp'; +if (file_exists($temp_file)) { + unlink($temp_file); // file should not exist before test +} + +$handle = fopen($temp_file, 'w'); + +curl_setopt($ch, CURLOPT_STDERR, $handle); +curl_setopt($ch, CURLOPT_VERBOSE, 1); +curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1); + +curl_exec($ch); + +fclose($handle); // causes glibc memory error + +//unlink($temp_file); // uncomment to test segfault (file not found on iowrite.c) + +curl_close($ch); +echo "Closed correctly\n"; +?> +--CLEAN-- +<?php +unlink(dirname(__FILE__) . '/curl_file_deleted_before_curl_close.tmp'); +?> +--EXPECTF-- +Closed correctly
-- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php