Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Bug #72828 »

Bug #72828

php.internals

Christoph Becker

10 years ago
Hi Nikita! You have commented on <https://bugs.php.net/bug.php?id=72828>: | Unless the allocations explicitly use the system allocator (i.e. do | not use emalloc and variants), do NOT introduce NULL checks. Can you please elaborate, why that shouldn't be done. Actually, the allocations use safe_emalloc() and emalloc(), respectively[1]. However, the only client of the function does explicitly check for a NULL return[2], which can only happen, if safe_emalloc() fails. So if no NULL checks should be done, this one should be removed as well. [1] <https://github.com/php/php-src/blob/php-5.6.24/ext/standard/string.c#L2926-L2927> [2] <https://github.com/php/php-src/blob/php-5.6.24/ext/standard/string.c#L3133-L3134>
-- Christoph M. Becker

Nikita Popov

10 years ago
On Sat, Aug 13, 2016 at 2:42 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> Hi Nikita! > > You have commented on <https://bugs.php.net/bug.php?id=72828>: > > | Unless the allocations explicitly use the system allocator (i.e. do > | not use emalloc and variants), do NOT introduce NULL checks. > > Can you please elaborate, why that shouldn't be done. > > Actually, the allocations use safe_emalloc() and emalloc(), > respectively[1]. However, the only client of the function does > explicitly check for a NULL return[2], which can only happen, if > safe_emalloc() fails. So if no NULL checks should be done, this one > should be removed as well. > > [1] > <https://github.com/php/php-src/blob/php-5.6.24/ext/ > standard/string.c#L2926-L2927> > [2] > <https://github.com/php/php-src/blob/php-5.6.24/ext/ > standard/string.c#L3133-L3134> >
If that's the only failure condition of the function, the check can indeed be dropped. ZMM is an infallible allocator, it will bail out (or abort) if the allocation fails. The return value should never be checked -- if it is checked, it's a mistake. If you are concerned about USE_ZEND_ALLOC=0 using a fallible allocator, the system allocator hooks in ZMM should be adjusted to check for NULL and abort instead (use __zend_malloc instead of malloc, for example). Nikita

Christoph Becker

10 years ago
On 13.08.2016 at 15:08, Nikita Popov wrote:
> On Sat, Aug 13, 2016 at 2:42 PM, Christoph M. Becker <cmbecker69@gmx.de> > wrote: > >> You have commented on <https://bugs.php.net/bug.php?id=72828>: >> >> | Unless the allocations explicitly use the system allocator (i.e. do >> | not use emalloc and variants), do NOT introduce NULL checks. >> >> Can you please elaborate, why that shouldn't be done. >> >> Actually, the allocations use safe_emalloc() and emalloc(), >> respectively[1]. However, the only client of the function does >> explicitly check for a NULL return[2], which can only happen, if >> safe_emalloc() fails. So if no NULL checks should be done, this one >> should be removed as well. >> >> [1] >> <https://github.com/php/php-src/blob/php-5.6.24/ext/ >> standard/string.c#L2926-L2927> >> [2] >> <https://github.com/php/php-src/blob/php-5.6.24/ext/ >> standard/string.c#L3133-L3134> > > If that's the only failure condition of the function, the check can indeed > be dropped.
Fine; I'll do that.
> ZMM is an infallible allocator, it will bail out (or abort) if the > allocation fails. The return value should never be checked -- if it is > checked, it's a mistake.
Thanks for the explanation!
> If you are concerned about USE_ZEND_ALLOC=0 using a fallible allocator, the > system allocator hooks in ZMM should be adjusted to check for NULL and > abort instead (use __zend_malloc instead of malloc, for example).
I don't know whether USE_ZEND_ALLOC=0 is in use on any production systems, but if that is so, that should be catered to. And maybe also to zero size allocations, which have implemention defined behavior (either NULL or a valid pointer may be returned).
-- Christoph M. Becker

Stas Malyshev

10 years ago
Hi!
> You have commented on <https://bugs.php.net/bug.php?id=72828>: > > | Unless the allocations explicitly use the system allocator (i.e. do > | not use emalloc and variants), do NOT introduce NULL checks. > > Can you please elaborate, why that shouldn't be done. > > Actually, the allocations use safe_emalloc() and emalloc(), > respectively[1]. However, the only client of the function does
These do not need null checks. If e* functions can't alloc memory, they produce fatal error, bail out and do not return. So if it returned, it succeeded. The only reason why it can return null is a bug. In this case we'd prefer it crashing fast - bugs in memory allocator are hard to find, and closer to the source the crash is, easier it to catch it. If pe* or system functions are used, then check may be warranted.
-- Stas Malyshev smalyshev@gmail.com

Christoph Becker

10 years ago
Hi! On 14.08.2016 at 10:21, Stanislav Malyshev wrote:
>> You have commented on <https://bugs.php.net/bug.php?id=72828>: >> >> | Unless the allocations explicitly use the system allocator (i.e. do >> | not use emalloc and variants), do NOT introduce NULL checks. >> >> Can you please elaborate, why that shouldn't be done. >> >> Actually, the allocations use safe_emalloc() and emalloc(), >> respectively[1]. However, the only client of the function does > > These do not need null checks. If e* functions can't alloc memory, they > produce fatal error, bail out and do not return. So if it returned, it > succeeded. The only reason why it can return null is a bug. In this case > we'd prefer it crashing fast - bugs in memory allocator are hard to > find, and closer to the source the crash is, easier it to catch it. > > If pe* or system functions are used, then check may be warranted.
Thanks! I had already closed the bug report, because the issue affects only PHP 5.6, and I don't think this extraneous NULL check is a real bug. Please re-open if you don't agree, in which case I'll remove the NULL check.
-- Christoph M. Becker