Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:52477
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain nebm.ist.utl.pt from 193.136.128.22 cause and error)
Content-Type: text/plain; charset=utf-8; format=flowed; delsp=yes
To: internals@lists.php.net
References: <BANLkTinNZmE9NszP4nqeGpx3ox0eBWx6RA@mail.gmail.com>
 <op.vvmp6xm9idpuyk@clk-0081.clk-domain>
 <BANLkTincengUnyU4Zz6i0e_hM+WHKufykg@mail.gmail.com>
 <BANLkTinNu8LUZtnJRBvzXk9o1ykLeE1Dsg@mail.gmail.com>
Date: Fri, 20 May 2011 12:12:14 +0100
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Organization: =?utf-8?Q?N=C3=BAcleo_de_Eng=2E_Biom=C3=A9di?=
 =?utf-8?Q?ca_do_I=2ES=2ET=2E?=
Message-ID: <op.vvr1eo2gidpuyk@clk-0081.clk-domain>
In-Reply-To: <BANLkTinNu8LUZtnJRBvzXk9o1ykLeE1Dsg@mail.gmail.com>
User-Agent: Opera Mail/11.10 (Win32)
Subject: Re: [PHP-DEV] Need karma for committing test patches
From: glopes@nebm.ist.utl.pt ("Gustavo Lopes")

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