I think you're a little optimistic about how effective these macros would
be for overflow checks. Also, if we're talking ANSI C or C99, then size_t
is always unsigned, and as far as I know GCC 2.4 always treats it as such.
If we're trying to stick to C here anyway.

As far as architecture specific stuff I would much rather rely on using the
built-in GCC overflow checks here
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

... as they are much safer and likely going to be far more performant than
doing all these casts everywhere. Not to mention the fact that you can
actually catch the overflow at the actual arithmetic level, where it's
safe, and hopefully be able to rely on the ISA's overflow or carry bits. If
we're trying to detect overflows or wraps after the fact, you don't add
much in the way of security. For example, I'm not at all sure how (zlong) <
(zend_long)INT_MIN will ever detect an overflow.

On Fri, Aug 21, 2015 at 5:40 AM, Anatol Belski anatol.php@belski.net
wrote:

-----Original Message-----
From: Anatol Belski [mailto:anatol.php@belski.net]
Sent: Friday, August 21, 2015 11:38 AM
To: 'Sherif Ramadan' theanomaly.is@gmail.com
Cc: '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;
'Matt Wilmas' php_lists@realplain.com; 'PHP Internals'
internals@lists.php.net
Subject: RE: [PHP-DEV] Overflow checks and integral vars comparison

Hi Sherif,

-----Original Message-----
From: Sherif Ramadan [mailto:theanomaly.is@gmail.com]
Sent: Friday, August 21, 2015 11:21 AM
To: Anatol Belski anatol.php@belski.net
Cc: 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; Matt Wilmas php_lists@realplain.com; PHP
Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Overflow checks and integral vars comparison

Maybe I'm missing something here, but how do these macros detect
overflow exactly? If the check is done on the actual result and not
the operands then it's not a good overflow check. Additionally, why
wouldn't overflow checks be needed on 32-bit architecture, or any other
architecture for that matter?
Integers can overflow there too.

Example code in simplexml_load_string()

if (zend_parse_parameters(ZEND_NUM_ARGS(), "s|C!lsb", &data,

&data_len,

&ce, &options, &ns, &ns_len, &isprefix) == FAILURE) {
return;
}

If (ZEND_LONG_INT_OVFL(options)) {
     RETURN_FALSE;
}
If (ZEND_SIZE_T_INT_OVFL(data_len)) {
     RETURN_FALSE;
}

docp = xmlReadMemory(data, data_len, NULL, NULL, options);

• on x86_64 - possible int overflow without check
• on ILP64 or i386 alike - no int overflow per se, so can be ommited

No int overflow with zend_long I wanted to say, so it's "if (0)" which is
eliminated, but with size_t an overflow is still possible.

On Fri, Aug 21, 2015 at 4:41 AM, Anatol Belski anatol.php@belski.net
wrote:

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

• standardize the overflow checks
• do actualy checks only on architectures where it's needed
• 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

#define ZEND_SIZE_T_INT_OVFL(size) ((size) > (size_t)INT_MAX)

So having it like

If (ZEND_LONG_INT_OVFL(x)) {
return;
}

Compiler would eliminate the branch automatically on 32-bit and
ILP64.

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))

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?

Thanks

Anatol

--
To unsubscribe,
visit: http://www.php.net/unsub.php

--
To unsubscribe,
visit:
http://www.php.net/unsub.php

-----Original Message-----
From: Matt Wilmas [mailto:php_lists@realplain.com]
Sent: Tuesday, August 25, 2015 2:19 PM
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
Subject: Re: [PHP-DEV] Overflow checks and integral vars comparison

But it is definitely fine to be optimistic and rely on compiler to do
basic, basic
stuff like this. No reason to make things more complicated just to think
one is
helping the compiler.

Proof? Have you seen the whole VM in the last, like, 10 years? Using
your logic,
we have major, major, MAJOR problems literally everywhere there!

Now can anyone point to problems compiling the default, specialized VM? I
rest
my case. :-) There's conditional stuff ALL over the place (not macros),
and
propagated to inline functions, etc. where the compiler is "counted on" to
remove the constant conditions and all that stuff.

The current FAST_ZPP parameter parsing, any unnecessary parts are
optimized
out (it'd be bad if that wasn't the case). My new proposal (coming
soon...) has a
lot of more complicated-looking source code, but almost all deleted by the
compiler.

Transforming zpp's type spec string at compile time, which MSVC can't do
because it does indeed suck for some reason, involves dozens of KB of
source,
many inline functions, etc. but nearly 100% of it is deleted, correctly,
by GCC and
Clang.

Anyway, that's why I say if "some" compiler can't do basic optimizations,
it
sucks, and has much bigger problems in the rest of the source, so
there's no
reason to worry about it... At all.

An #ifdef seems just a bit more readable, less tricky. Clear if a compiler
tells it implements some standard like C89, it should be trustful. Or a
feature like dead code elimination. Some projects even use not standard
compiler features depending on which platform they are in much bigger extent
than PHP. My sentence about not being optimistic is about not trusting
blindly on that, not ignoring useful things.

I mean, PHP's strlen() on any platform, why is it possible to have larger
strings
(size_t length) than can be returned in zend_long. That's a bug.

memory_limit is unsigned 32-bit there, but even if not - it is hardly
possible to have a string exceeding it there anyway.

Not possible? Then like I said, why use size_t? It would help some of
these
things to get rid of it.

It is a "theoretical" bug. No issue on 32-bit, on 64-bit you would need to
have a system with over 10^9 GB of RAM to come to it :)

Otherwise, there is a plenty of libraries working with size_t and it
makes full sense to exhaust that on 64-bit. And the most of PHP7 is
just fine with size_t.

So? Why does it matter if libraries are using size_t? The same libraries
always
have been and passed an int (from string length) before PHP 7. Pass them
zend_long instead now...

One aspect is because of 64-bit, to exhaust the potential of the platform.
But it is again more general discussion whether PHP is a general purpose or
web dedicated language.

Regards

Anatol

I didn't reply in last week's thread about the overflow checks in
OpenSSL...

But it is definitely fine to be optimistic and rely on compiler to do
basic, basic stuff like this. No reason to make things more complicated
just to think one is helping the compiler.

Proof? Have you seen the whole VM in the last, like, 10 years? Using
your logic, we have major, major, MAJOR problems literally everywhere there!

Now can anyone point to problems compiling the default, specialized VM?
I rest my case. :-) There's conditional stuff ALL over the place (not
macros), and propagated to inline functions, etc. where the compiler is
"counted on" to remove the constant conditions and all that stuff.

The current FAST_ZPP parameter parsing, any unnecessary parts are
optimized out (it'd be bad if that wasn't the case). My new proposal
(coming soon...) has a lot of more complicated-looking source code, but
almost all deleted by the compiler.

Transforming zpp's type spec string at compile time, which MSVC can't do
because it does indeed suck for some reason, involves dozens of KB of
source, many inline functions, etc. but nearly 100% of it is deleted,
correctly, by GCC and Clang.

Anyway, that's why I say if "some" compiler can't do basic optimizations,
it sucks, and has much bigger problems in the rest of the source, so
there's no reason to worry about it... At all.

Let me disagree on this comment. Most of it and its wording.

Also it is probably a question of taste. SIZEOF_INT == SIZEOF_ZEND_LONG
is
something with zero calculation and guaranteed to exclude any possible
side
effect, while sizeof(int) == sizeof(zend_long) depends on the
optimization
choosen. Not disputing that your suggestion will do nearly the same job
in
the usual case, ofc.

With the casts - probably yes, many cases will go by the standard type
promotion rules, but having the casts only ensures it does what one
actually
intended to do and has no effect in the generated code. For example one
would need a cast when comparing with INT_MAX + 1. Worse is with
unsigned,
because the result depends on the platform and concrete types and values.

Hiding behind the macros makes full sense for two reasons at least

• there are people with various skills that can just rely on what the API
  provides without going too deep into the detail
• doing an internal change won't affect anything in the existing usages

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...

Not sure what you mean here, 32-bit?

I mean, PHP's strlen() on any platform, why is it possible to have larger
strings (size_t length) than can be returned in zend_long. That's a bug.

It is wrong to use long for portability. It is also wrong (very) to use
int* for length of a buffer. It is a very well known good practice (correct
way) to use size_t, even more when interacting with external sources.

I have to disagree about your statement here too.

So? Why does it matter if libraries are using size_t? The same
libraries always have been and passed an int (from string length) before
PHP 7. Pass them zend_long instead now...

And we have issues because of this exact case. It also helps to use the
right type for a given usage. In this case size_t.

Before the 64-bit RFC my idea was actually to port the dependency libs
for
the full 64-bit support, but it's insane. So better to fix the side
effects,
as those are almost the same as in PHP5 on most of 64-bit.

Regards

Anatol

• Matt