Fix #76342: file_get_contents waits twice specified timeout #4486
Conversation
fancyweb
force-pushed
the
http-fopen-wrapper-timeout
branch
from
691c6d3 to
ba5648d
Compare
fancyweb
force-pushed
the
http-fopen-wrapper-timeout
branch
from
ba5648d to
86e6cff
Compare
| @@ -726,6 +726,11 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, | |||
| } | |||
| ZVAL_STRINGL(&http_response, tmp_line, tmp_line_len); | |||
| zend_hash_next_index_insert(Z_ARRVAL_P(response_header), &http_response); | |||
| } else { | |||
| php_stream_close(stream); | |||
| stream = NULL; | |||
There was a problem hiding this comment.
While merging this up into 7.4, I found that we have 41c4816 there, which should have much the same effect -- the main difference I see is that it does not close and null the stream. The EOF branch below also doesn't do this. I'm wondering if it's necessary?
There was a problem hiding this comment.
I did this to be coherent with lines 851 & 852 (
php-src/ext/standard/http_fopen_wrapper.c
Line 851 in 86e6cff
Removing both lines doesn't work because the stream is not technically EOF, so it still goes through the second call, thus applying 2 times the timeout.
Only php_stream_close(stream) -> seg fault
Only stream = null -> seems to work
|
Sorry for the delay, I forgot about this. Now merged as e691a98 into 7.2+. Thanks! |
Currently, if getting the response headers fails, we are still trying to read past them to get the content. Is that logical? 🤔
That's the cause of the fixed bug : the current behavior makes 2 call to
php_stream_get_line, thus "applying" 2 times the timeout. I guess that in the particular described setup, the end of stream is not detected. However, the timeout being applied 2 times is not coherent at all.I just moved the
/* read past HTTP headers */code block so it is executed only if getting the headers was successful. We can also fix it by forcing$stream->eof = 1;when the first call fails but I guess that's dirtier.I know close to nothing about C and PHP source code but at least the test can be used if it can be fixed in a better way 😄