Help - gcc mystery in ext/imap

9 years ago by Rasmus Lerdorf — view source

unread

In testing PHP7 beyond just the default extensions, I am noticing that
ext/imap is segfaulting on almost every test. I tracked it down to this
code in ext/imap/php_imap.c:

/* {{{ imap_do_open
*/
static void php_imap_do_open(INTERNAL_FUNCTION_PARAMETERS, int persistent)
{
char *mailbox, *user, *passwd;
int mailbox_len, user_len, passwd_len;
long retries = 0, flags = NIL, cl_flags = NIL;
MAILSTREAM *imap_stream;
pils *imap_le_struct;
zval *params = NULL;
int argc = ZEND_NUM_ARGS();

    if (zend_parse_parameters(argc, "pss|lla", &mailbox,

&mailbox_len, &user, &user_len,
&passwd, &passwd_len, &flags, &retries, &params) ==
FAILURE) {
return;
}
...
/* local filename, need to perform open_basedir check */
if (mailbox[0] != '{' && php_check_open_basedir(mailbox)) {
RETURN_FALSE;
}

The actual segfault is on the access to mailbox[0] because mailbox is a
bogus pointer at this point and from userspace it is reproducable from
cli with just:

sapi/cli/php -r 'imap_open("host", "user", "password");'

And here is where it gets mysterious. Looking at it with gdb,
zend_parse_parameters is setting that mailbox char* to 0x7fff00000000
while both the user and passwd ptrs are fine. BUT, if I recompile
ext/imap/php_imap.c using gcc -O3 the ptr is magically fine and no more
segfaults.

I suspected my gcc-4.8 on my Ubuntu laptop, so I built a completely
clean Debian 7.8.0 box which defaults to gcc-4.7.2 and I see the exact
same behaviour. segfaults under -O1/O2 but it is fine under -O0/O3. It
is also fine from the PHP 5.6 tree even though the changes are mostly
just removing TSRM and moving from zval** to zval* in a couple of places.

Things I have tried. changed it from "p" to "s" and also from "p" to "S"
and using a zend_string * instead. In both cases I got the same bogus
address back from zpp. Right now I am trying to make sense of the
generated assembly differences, but it is slow going.

Since almost the same code works for PHP 5.6 and this is limited to just
the imap extension, it seems more logical that something in the 5-6 to 7
changes is causing this and somehow -O3 optimizes its way around the
problem. So, does anyone see a mistake in these changes?

https://gist.github.com/anonymous/b92dbb17172e9bff5247

-Rasmus

9 years ago by Rasmus Lerdorf — view source

unread

Things I have tried. changed it from "p" to "s" and also from "p" to "S"
and using a zend_string * instead. In both cases I got the same bogus
address back from zpp. Right now I am trying to make sense of the
generated assembly differences, but it is slow going.

It looks like I might have messed up the zend_string try before. Both
Nikic and Michael suggested it probably had to to with int instead of
size_t for the arg lengths, which I had considered but figured since it
also broke with a zend_string which brings along the right length type
that wasn't it. But, re-applying my patch:

https://gist.github.com/anonymous/713089b9acca67bce42e

seems to have fixed it.

So, I guess the lesson is that we need to be careful when we migrate
extensions to PHP 7. Passing ints instead of size_t string length params
to zpp can cause extreme weirdness and we should go through all our
bundled extensions and make sure things that are still using "s" have
all been changed to size_t.

-Rasmus

9 years ago by johannes@schlueters.de — view source

unread

So, I guess the lesson is that we need to be careful when we migrate
extensions to PHP 7. Passing ints instead of size_t string length params
to zpp can cause extreme weirdness and we should go through all our
bundled extensions and make sure things that are still using "s" have
all been changed to size_t.

There is this plugin to clang's static analyzer which should help:
https://github.com/johannes/clang-php-checker

It might needs updates for current types, though. Relevant code place to
update:
https://github.com/johannes/clang-php-checker/blob/master/PHPZPPChecker.cpp#L67

johannes

9 years ago by Stanislav Malyshev — view source

unread

Hi!

So, I guess the lesson is that we need to be careful when we migrate
extensions to PHP 7. Passing ints instead of size_t string length params
to zpp can cause extreme weirdness and we should go through all our
bundled extensions and make sure things that are still using "s" have
all been changed to size_t.

Definitely so. I've recently went through intl and fixed a bunch of
int/size_t issues, but judging from how many there were, I assume other
non-trivial extensions would have such things too. We have lots of
places where size_t and int are intermixed - e.g., random try, go to
mysqli extension, do grep int *.c | grep len and see that results of
spprintf - which returns size_t - are put into int. Now, in this case
it's almost 100% not a problem, but illustrates bigger issue of mixing
the types, and in some cases it may be a real problem.

Stas Malyshev
smalyshev@gmail.com

9 years ago by Rasmus Lerdorf — view source

unread

Hi!

So, I guess the lesson is that we need to be careful when we migrate
extensions to PHP 7. Passing ints instead of size_t string length params
to zpp can cause extreme weirdness and we should go through all our
bundled extensions and make sure things that are still using "s" have
all been changed to size_t.

Definitely so. I've recently went through intl and fixed a bunch of
int/size_t issues, but judging from how many there were, I assume other
non-trivial extensions would have such things too. We have lots of
places where size_t and int are intermixed - e.g., random try, go to
mysqli extension, do grep int *.c | grep len and see that results of
spprintf - which returns size_t - are put into int. Now, in this case
it's almost 100% not a problem, but illustrates bigger issue of mixing
the types, and in some cases it may be a real problem.

Yeah, I was quite surprised by the weirdness it caused. I don't think I
have ever seen something that breaks under -O2 start working again
compiled with -O3. That's what had me chasing my tail into assembly
world for a bit there.

I have fixed up ext/imap now. Hopefully I caught them all.

-Rasmus

9 years ago by Yasuo Ohgaki — view source

unread

Hi all,

Hi!

So, I guess the lesson is that we need to be careful when we migrate
extensions to PHP 7. Passing ints instead of size_t string length params
to zpp can cause extreme weirdness and we should go through all our
bundled extensions and make sure things that are still using "s" have
all been changed to size_t.

Definitely so. I've recently went through intl and fixed a bunch of
int/size_t issues, but judging from how many there were, I assume other
non-trivial extensions would have such things too. We have lots of
places where size_t and int are intermixed - e.g., random try, go to
mysqli extension, do grep int *.c | grep len and see that results of
spprintf - which returns size_t - are put into int. Now, in this case
it's almost 100% not a problem, but illustrates bigger issue of mixing
the types, and in some cases it may be a real problem.

Yeah, I was quite surprised by the weirdness it caused. I don't think I
have ever seen something that breaks under -O2 start working again
compiled with -O3. That's what had me chasing my tail into assembly
world for a bit there.

I have fixed up ext/imap now. Hopefully I caught them all.

I'm observing somewhat similar behavior. I don't have clue why it does...

gcc version 4.9.2 20141101 (Red Hat 4.9.2-1) (GCC)

Should we use size_t/ptrdiff_t everywhere applicable?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Stanislav Malyshev — view source

unread

Hi!

Should we use size_t/ptrdiff_t everywhere applicable?

We should use it everywhere where we deal with zend_string and such -
i.e. where the size_t parameter is expected. If it's related to outside
library - we should use its type and check (e.g. ICU uses int32_t). For
internal things, I guess it goes on case-by-case basis, depending on how
big the thing in question can get.

--
Stas Malyshev
smalyshev@gmail.com