fix #78326 memory issues with stream_get_contents() fixed length buffer#4464
fix #78326 memory issues with stream_get_contents() fixed length buffer#4464acasademont wants to merge 1 commit intophp:PHP-7.2from
Conversation
|
|
| *ptr = '\0'; | ||
| ZSTR_LEN(result) = len; | ||
| result = zend_string_truncate(result, len, persistent); | ||
| ZSTR_VAL(result)[len] = '\0'; |
There was a problem hiding this comment.
You might want to use the same logic as in https://github.com/php/php-src/blob/master/main/streams/streams.c#L759 and only truncate if the "wastage" is large enough.
There was a problem hiding this comment.
@nikic thanks! I guess that the potential savings of the truncate are not worth the extra performance costs of the function and memory copy calls if the "waste" is not large enough?
There was a problem hiding this comment.
This dropped the ZSTR_LEN(result) = len assignment, which is still needed in case we don't truncate. I'm surprised this doesn't cause test failures.
There was a problem hiding this comment.
@nikic you're right, it didn't I'm afraid, any ideas for a test that maybe we could add?
There was a problem hiding this comment.
fixed in any case
There was a problem hiding this comment.
<?php
$f = fopen('php://memory', 'rw');
fwrite($f, str_repeat('X', 1000));
fseek($f, 0);
var_dump(strlen(stream_get_contents($f, 1024)));
should show the issue.
There was a problem hiding this comment.
@nikic it did, thanks! I added the additional test just to be on the safe side
acasademont
force-pushed
the
bug78326
branch
2 times, most recently
from
cc82d91 to
42ddb03
Compare
|
Should I also include a NEWS.txt entry? |
No. This is supposed to be done by the "merger". |
… fixed length buffer
|
Merged as dc7aa22 into 7.2+. Thanks! |
6 participants
Hi!
This is my first contribution, all tests keep passing, but I'm not sure the fix is 100% correct. Thanks!!
Fixes https://bugs.php.net/bug.php?id=78326