Re: [ZEND-ENGINE-CVS] cvs: ZendEngine2 / zend_constants.c

At 01:07 04/06/2003, Marcus Börger wrote:

At 14:13 03.06.2003, Zeev Suraski wrote:

At 21:45 02/06/2003, Andi Gutmans wrote:

Can you please explain the problem? This code has worked for ages so maybe we introduced a bug someplace else?

It does appear as if the code was buggy, even though I don't see how it could have any negative effect. It was just allocating one extra byte for no reason. If there were any real bugs this was supposed to fix - then it does seem like a voodoo fix. We need to see some reprodicuble crash to determine where it really originates (I could be wrong in my analysis, this may actually be the source of some problem). Marcus - our "no voodoo fixes" is a real, strict rule. It's fine that you think it's a bad rule, but it's still there. If you have a fairly good understanding that you're actually fixing a symptom, and not the problem itself - do not commit a patch. Consult the engine2 list instead. We prefer to go on crashing and be able to find the real bug than covering it, and having to go nuts when we actually try to find it in the future.

Hehe Voodo-Fix sounds nice for a problem that took me hours to track down before using valgrind which simply told me that the is a memory overrun.

valgrind is a nice tool but it's still not the voice of God. A memory overrun or an uninitialized memory read is a *symptom*, not a source of a problem. In this particular case, it's much more likely that the code that was touching EG(uninitialized_zval) shouldn't have been doing it. As such the early initialization does appear to be a voodoo fix, and the real fix would be determining why EG(uninitialized_zval) is even being touched at this stage.

Ok i corrected two things: I added the the following line

        EG(uninitialized_zval_ptr)=&EG(uninitialized_zval);

Removing it again from 4.3.2/ZE1 shows this: marcus@zaphod /usr/src/PHP_4_3_0 $ make sapi/cli/php && valgrind --num-callers=20 --workaround-gcc296-bugs=yes sapi/cli/php -r "" make: `sapi/cli/php' is up to date. ==12996== valgrind-1.0.4, a memory error detector for x86 GNU/Linux. ==12996== Copyright (C) 2000-2002, and GNU GPL'd, by Julian Seward. ==12996== Estimated CPU clock rate is 456 MHz ==12996== For more details, rerun with: -v ==12996== ==12996== Conditional jump or move depends on uninitialised value(s)

==12996==    at 0x821D7AA: _zval_ptr_dtor (/usr/src/PHP_4_3_0/Zend/zend_execute.h:43)
==12996==    by 0x81C1C8A: OnChangeCallback (/usr/src/PHP_4_3_0/ext/standard/assert.c:57)
==12996==    by 0x8230D5E: zend_ini_refresh_cache (/usr/src/PHP_4_3_0/Zend/zend_ini.c:176)
==12996==    by 0x822BA0B: zend_hash_apply_with_argument (/usr/src/PHP_4_3_0/Zend/zend_hash.c:707)
==12996==    by 0x8230D9A: zend_ini_refresh_caches (/usr/src/PHP_4_3_0/Zend/zend_ini.c:184)
==12996==    by 0x8230B4A: zend_copy_ini_directives (/usr/src/PHP_4_3_0/Zend/zend_ini.c:103)
==12996==    by 0x82260C0: zend_new_thread_end_handler (/usr/src/PHP_4_3_0/Zend/zend.c:375)
==12996==    by 0x82264C9: zend_post_startup (/usr/src/PHP_4_3_0/Zend/zend.c:546)
==12996==    by 0x81F89A4: php_module_startup (/usr/src/PHP_4_3_0/main/main.c:1241)
==12996==    by 0x824310D: main (/usr/src/PHP_4_3_0/sapi/cli/php_cli.c:575)
==12996==    by 0x40A60280: __libc_start_main (in /lib/libc-2.2.4.so)
==12996==    by 0x8083081: png_read_end (in /usr/src/PHP_4_3_0/sapi/cli/php)

==12996== ==12996== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 9 from 4) ==12996== malloc/free: in use at exit: 340419 bytes in 9223 blocks. ==12996== malloc/free: 17363 allocs, 8140 frees, 934114 bytes allocated. ==12996== For a detailed leak analysis, rerun with: --leak-check=yes ==12996== For counts of detected errors, rerun with: -v So this is really no voodo.

For instance, here the bug is in the assert code, which shouldn't be using MAKE_STD_ZVAL() outside a request. It relies on the reference counting mechanisms which aren't initialized at this stage. Instead, it should be a malloc'd piece of memory allocated and initialized manually.

I also changed ZEND_STRL to ZEND_STRS in ZE1 and ZE2 to be consistent with the other places and ways where constants are added. Actually i didn't find a problem here using valgrind

I actually think this should be cleaned, since it's just faulty right now. I was worried that this was voodooish since this shouldn't have any symptoms, but if you've just been cleaning things - it should be fine.

The next thing i did which i will not revert is the following diff (notice the versions): RCS file: /repository/ZendEngine2/zend_constants.c,v retrieving revision 1.48 retrieving revision 1.51 diff -u -p -r1.48 -r1.51

--- Zend/zend_constants.c       21 May 2003 22:57:51 -0000      1.48
+++ Zend/zend_constants.c       1 Jun 2003 16:20:23 -0000       1.51@@ -244,11 +244,12 @@ ZEND_API int zend_register_constant(zend
        printf("Registering constant for module %d\n", c->module_number);

#endif

-       lowercase_name = do_alloca(c->name_len + 1);

-

        if (!(c->flags & CONST_CS)) {
-               zend_str_tolower_copy(lowercase_name, c->name, c->name_len);
+               /* keep in mind that c->name_len already contains the '\0' */
+               lowercase_name = do_alloca(c->name_len);
+               zend_str_tolower_copy(lowercase_name, c->name, c->name_len - 1);
        } else {
+               lowercase_name = do_alloca(c->name_len + 1);
                memcpy(lowercase_name, c->name, c->name_len);
                lowercase_name[c->name_len] = '\0';
        }

As you might see by taking a closer look the first variant used to do a memory overrun. Play with valgrind a bit.

This looks somewhat messy (allocating different sizes depending on the if?). This whole off-by-one thing should definitely be cleaned up. But it's not urgent, since it works. I would change zend_str_tolower_copy() to assign 0 to *result instead of assigning *end - slightly quicker, and would make it binary safe (i.e., not require a NULL at the end).

p.s.: I am not stupid. I don't do voodo-tricks. Those i do in SPL :-)

I don't think anybody thinks you're stupid. I definitely don't. But even smart people do voodoo fixes, we just pointed out that we're not very receptive to them in PHP and in the engine in particular :) Zeev