Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:55662 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 45966 invoked from network); 29 Sep 2011 12:14:56 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 29 Sep 2011 12:14:56 -0000 Authentication-Results: pb1.pair.com smtp.mail=olivier@yakaz.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=olivier@yakaz.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain yakaz.com designates 209.85.216.177 as permitted sender) X-PHP-List-Original-Sender: olivier@yakaz.com X-Host-Fingerprint: 209.85.216.177 mail-qy0-f177.google.com Received: from [209.85.216.177] ([209.85.216.177:50279] helo=mail-qy0-f177.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id D6/10-44577-F31648E4 for ; Thu, 29 Sep 2011 08:14:56 -0400 Received: by qyk31 with SMTP id 31so718679qyk.8 for ; Thu, 29 Sep 2011 05:14:52 -0700 (PDT) Received: by 10.229.82.76 with SMTP id a12mr4336786qcl.110.1317298491166; Thu, 29 Sep 2011 05:14:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.229.190.199 with HTTP; Thu, 29 Sep 2011 05:14:31 -0700 (PDT) Date: Thu, 29 Sep 2011 14:14:31 +0200 Message-ID: To: internals@lists.php.net Content-Type: text/plain; charset=ISO-8859-1 Subject: Cast trap with INI entries compiling C++ extension From: olivier@yakaz.com (Olivier Favre) Hi everyone, I've been developing a PHP extension for internal needs. We're using C++, by using PHP_REQUIRE_CXX() in config.m4. I'm using debian sid 64bits, with the package php5-dev-5.3.8-2 (against which the patch below has been created). When declaring an INI entry, like this: PHP_INI_BEGIN() STD_PHP_INI_ENTRY("extensionname.variable1", NULL, PHP_INI_ALL, OnUpdateString, globalsVariable1, zend_extensionname_globals, extensionname_globals) STD_PHP_INI_ENTRY("extensionname.variable2", NULL, PHP_INI_ALL, OnUpdateString, globalsVariable2, zend_extensionname_globals, extensionname_globals) PHP_INI_END() We get the following warning: warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings] I have a bunch of those, one per INI entry, and prefer to get rid of them. As a usual fix, I changed the line to use char* C-style cast as follows: STD_PHP_INI_ENTRY((char*)"extensionname.variable1", NULL, PHP_INI_ALL, OnUpdateString, globalsVariable1, zend_extensionname_globals, extensionname_globals) STD_PHP_INI_ENTRY((char*)"extensionname.variable2", NULL, PHP_INI_ALL, OnUpdateString, globalsVariable2, zend_extensionname_globals, extensionname_globals) However, this does not work, I can see through phpinfo(), listing the extension's INI entries, that only "extensi" is listed. I'm aware that const char* should be used instead, but even using strdup("extensionname.variableN") does not work! It took me a while to find the problem. STD_PHP_INI_ENTRY resolves to STD_ZEND_INI_ENTRY which resolves to ZEND_INI_ENTRY2, then ZEND_INI_ENTRY2_EX then in turn ZEND_INI_ENTRY3_EX which finally uses the following with the first macro parameter "name": [...], name, sizeof(name), [...] Yep, the problem comes from the sizeof(), which works on a string constant seemingly interpreted as a (const?) char[] in C and C++, returning sizeof(void*) instead of the length of the string (+ the trailing \0), when using the classic char* cast. Fine as this is probably not the right cast (it is really a very common mistake), but just before we are setting a field of type char*, which is the one that raises the warning. My point is that there is a problem if it is that easy to trigger a bug with some "natural reflex" to get rid of a warning. I suggest some fixes: * Use strlen() instead of sizeof(). * Use in-macro cast to char[]. * Use const when the string value won't be modified (I'm not talking about the pointer, but its content). In fact, I propose the following changes so that no user (extension writer) code has to change: --- zend_ini.h 2011-09-29 12:24:39.012882527 +0200 +++ zend_ini.h 2011-09-29 12:24:32.552577965 +0200 @@ -65,14 +65,14 @@ struct _zend_ini_entry { int module_number; int modifiable; - char *name; + const char *name; uint name_length; ZEND_INI_MH((*on_modify)); void *mh_arg1; void *mh_arg2; void *mh_arg3; - char *value; + const char *value; uint value_length; char *orig_value; @@ -117,7 +117,7 @@ #define ZEND_INI_END() { 0, 0, NULL, 0, NULL, NULL, NULL, NULL, NULL, 0, NULL, 0, 0, 0, NULL } }; #define ZEND_INI_ENTRY3_EX(name, default_value, modifiable, on_modify, arg1, arg2, arg3, displayer) \ - { 0, modifiable, name, sizeof(name), on_modify, arg1, arg2, arg3, default_value, sizeof(default_value)-1, NULL, 0, 0, 0, displayer }, + { 0, modifiable, name, strlen(name)+1, on_modify, arg1, arg2, arg3, default_value, default_value != NULL ? strlen(default_value) : sizeof(NULL), NULL, 0, 0, 0, displayer }, #define ZEND_INI_ENTRY3(name, default_value, modifiable, on_modify, arg1, arg2, arg3) \ ZEND_INI_ENTRY3_EX(name, default_value, modifiable, on_modify, arg1, arg2, arg3, NULL) We use const char* fields not to trigger the C++ deprecation warning, and we use strlen() to get the size of the string (which is the only normal way anyway), but test for a non NULL value (useless for "name" I guess). By the way, I still return sizeof(NULL) for compatibility, but 0 should probably be a better value. I only tested that change with building my C++ PHP extension, whole PHP recompilation. Best, --- Olivier Favre Software engineer Yakaz http://www.yakaz.com