Re: Need karma for committing test patches

Em Fri, 20 May 2011 07:59:51 +0100, Alexey Shein <confik@gmail.com> escreveu:

* 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?

First some considerations about your patch: * You seem to be leaking the FILE* variable you created (you just removed the zval_ptr_dtor(&ch->handlers->std_err); did not add any fclose call). * You say the problem also occurs with CURLOPT_WRITEHEADER, CURLOPT_WRITEHEADER and CURLOPT_INFILE. But again, you don't consider the curl extension doesn't close the stdio pointers. * No error checking in the calls to fileno, dup or fdopen. * php_stream.mode cannot be used directly; see php_stream_mode_sanitize_fdopen_fopencookie. Now, I'm starting to think something in the line of your original patch is better. Of course, curl_multi_exec and the other options would have to be considered too. The reason is I hadn't considered the curl extensions casts into a FILE*, not an fd. Sure, sometimes you can still dup the fd and create another stdio pointer, but this is not always the case (see fopencookie). So this represents a removal of some functionality. Another solution would be to force the stream to preserve its handle upon closing. This is possible with PHP_STREAM_FLAG_NO_CLOSE, but I don't think throwing the actual file closing to the script shutdown is a good idea. -- Gustavo Lopes