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

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. 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. Thies already mentioned that it may not fix the problems in all sapi modules. But the sapis work different and are initialized different. So i focused on fixing some CLI/CGI related problems. That is also the reason why i left in the original place of that initialization line: to be reminded where it came from - just like you keep your bugs. 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 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. p.s.: I am not stupid. I don't do voodo-tricks. Those i do in SPL :-)