Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87900
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain realplain.com from 216.33.127.81 cause and error)
Message-ID: <A6C1D55B475340ACB6D42434C270DEEE@pc1>
To: "Anatol Belski" <anatol.php@belski.net>,
	"'Dmitry Stogov'" <dmitry@php.net>,
	"'Xinchen Hui'" <xinchen.h@zend.com>,
	"'Nikita Popov'" <nikita.ppv@gmail.com>,
	"'Pierre Joye'" <pierre.php@gmail.com>,
	"'Bob Weinand'" <bobwei9@hotmail.com>,
	"'Jakub Zelenka'" <bukka@php.net>
Cc: <internals@lists.php.net>
References: <02a601d0dbed$2c828df0$8587a9d0$@belski.net>
Date: Tue, 25 Aug 2015 05:06:12 -0500
MIME-Version: 1.0
Content-Type: text/plain;
	format=flowed;
	charset="iso-8859-1";
	reply-type=original
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Overflow checks and integral vars comparison
From: php_lists@realplain.com ("Matt Wilmas")

Hi Anatol, Dmitry, all,

----- Original Message -----
From: "Anatol Belski"
Sent: Friday, August 21, 2015

> Hi,
>
> Resending this as missed internals at the start.
>
> I was lately rethinking some part of the 64-bit RFC, and also seeing now
> Jakub's work on catching overflows in ext/openssl and Matt Williams
> suggestions on it (which was going a bit more global over it). So I came 
> up
> with these macros with two goals

Dang, people be gettin' my name wrong, in the same way, even "in writing." 
;-P

> - standardize the overflow checks
> - do actualy checks only on architectures where it's needed

The compiler will already do that, as I've said of course, and more 
importantly (for most cases), it will never get it wrong.

> - simplify the checks where external libs (openssl, libxml, etc.) use firm
> datatypes like int
>
> #if SIZEOF_INT == SIZEOF_ZEND_LONG
> # define ZEND_LONG_INT_OVFL(zl) (0)
> # define ZEND_LONG_INT_UDFL(zl) (0)
> #else
> # define ZEND_LONG_INT_OVFL(zlong) ((zlong) > (zend_long)INT_MAX) # define
> ZEND_LONG_INT_UDFL(zlong) ((zlong) < (zend_long)INT_MIN) #endif

Really no point in all that...

> #define ZEND_SIZE_T_INT_OVFL(size) ((size) > (size_t)INT_MAX)

What's wrong with just doing the check, "as is," instead of hiding it behind 
a [longer] macro?  (Also, none of the casts are necessary...?)

> So having it like
>
> If (ZEND_LONG_INT_OVFL(x)) {
> return;
> }
>
> Compiler would eliminate the branch automatically on 32-bit and ILP64.

Already will. :^)

> Some other macros to do signed/unsigned comparison, these can be extended.
>
> #define ZEND_SIZE_T_GT_ZEND_LONG(size, zlong) ((zlong) < 0 || (size) >
> (size_t)(zlong)) #define ZEND_SIZE_T_GTE_ZEND_LONG(size, zlong) ((zlong) < 
> 0
> || (size) >= (size_t)(zlong)) #define ZEND_SIZE_T_LT_ZEND_LONG(size, 
> zlong)
> ((zlong) >= 0 && (size) < (size_t)(zlong)) #define
> ZEND_SIZE_T_LTE_ZEND_LONG(size, zlong) ((zlong) >= 0 && (size) <=
> (size_t)(zlong))

Yes, the only case where we might want to do something special is comparing 
an unsigned type, like size_t (string lengths), with a signed max, since 
that's not impossible to the compiler.  Of course, you have to KNOW that's 
what you want and/or it's safe to ignore the check, e.g. not dealing with 
strings > 2 GB on 32-bit, etc.

And again, a generic way to do that for a "max" of integer type or greater 
(since literals are smallest type >= int that will hold the value), that 
should work anywhere, is:

#define ZEND_SIGNED_OVFL_etc(val, max) (sizeof(val) > sizeof(max) && (val) > 
(max))

Simple and optimizes easily...

> IMHO these and maybe more are missing after the 64-bit RFC. Do you think
> they would make sense? Or would make sense now, or later in master?

Also, why, exactly, has the zend_string length been defined as size_t (just 
because it's "supposed to" be?), which is incompatible with most everything 
in PHP, instead of zend_long (since we had int before)?

e.g. Just look at strlen() -- oops, size_t no worky! :-/  Dmitry?  We need 
range checking there, right?  Conversion to double?  No?  Then there's *no 
point* in using size_t, and would let the compiler auto-optimize the 
above-referenced comparisons with size_t...

> Thanks
>
> Anatol

- Matt