User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041219 Firefox/1.0 (Debian package 1.0+dfsg.1-1) Build Identifier: gcc has the ability to check the (variable) argument lists of printf style functions against the given format string. This helps us to avoid buggy (and maybe crashing) code like int x=value; PR_smprintf("%s",x); Or think about int x; PR_smprintf("...%ld...",x,...); Which might cause real problems on systems with 64bit integers... Just to give some examples of issues, that are now found when compiling mozilla: pk11pars.h:764: warning: unsigned int format, long unsigned int arg (arg 2) nsPluginHostImpl.cpp:6339: warning: long int format, PRUint32 arg (arg 5) nsDefaultSOAPEncoder.cpp:3280: warning: int format, PRUint32 arg (arg 4) xpidl_util.c:143: warning: unsigned int format, PRInt32 arg (arg 3) prtime.c:1831: warning: long int format, int arg (arg 4) prinit.c:649: warning: long unsigned int format, int arg (arg 6) There is just one flaw: PR_sprintf (and the like) do not conform to ANSI X3.159-1989 (``ANSI C'') and ISO/IEC 9899:1999 (``ISO C99''). Because of that there come up some warnings that may be ignored but might be confusing. If I think about it: isn't it that modern C/C++ compilers allow to set the integer size (and long etc. sizes)? That would make PRUint32 etc. obsolete. Sounds like someday we have to rewrite the code comletely... ;-) Reproducible: Always Steps to Reproduce:

sorry, but the patch from bug #276483 is also included... hope, that isn't a blocker.

Tim, Thank you very much for contributing this patch. An engineer at Tellme Networks also contribtued this patch before. Unfortunately, I can't accept this patch because NSPR's printf format specifiers for integers are different from the C Standards. For example, Specifier NSPR C Standard =================================== %ld PRInt32 long %hd PRInt16 short The %ld discrepancy is problematic. PRInt32 is defined as 'int' on 32-bit platforms, which is not the same type as 'long'. On 64-bit platforms with the LP64 model, 'long' is 64-bit, which is different size from PRInt32.

Comment on attachment 170542 [details] [diff] [review] cvs diff It's really a shame that we can't take advantage of this useful gcc feature.

(In reply to Wan-Teh Chang from comment #2) > Tim, > > Thank you very much for contributing this patch. > > An engineer at Tellme Networks also contribtued this > patch before. Unfortunately, I can't accept this patch > because NSPR's printf format specifiers for integers > are different from the C Standards. For example, > > Specifier NSPR C Standard > =================================== > %ld PRInt32 long > %hd PRInt16 short What do you think of use %i and %hi for these?

hiro: we can't make changes to NSPR that will break existing code, unfortunately.

There are several possibilities to change NSPR without breaking existing code. The question is: what is the goal ? One answer is "Fixing 'misalignments' between the format and the arguments". One way to achieve that: - recompile all with gcc and format checking turned on and collecting all format warnigs - grep out and fix all warnings that might cause problems, like (from my first post) pk11pars.h:764: warning: unsigned int format, long unsigned int arg (arg 2) [here we should to fix the code, since %u expects a 32bit int. on LP64 the argument would be 64 bit. there might be a loss of information if the value is >= 2^32. is that wanted or not ?] It would be a good task for a "beginner" to sort the hard to solve things out and to fix the easy ones.

The problem is that the existing code that causes warnings may be correct. The format specifiers for NSPR are documented at http://www.mozilla.org/projects/nspr/reference/html/prprf.html#23299 It says: Between the percent and the conversion character there can be, in order: ... * An h if the integer is to be printed as a 16 bit value, a l if 32 bits, or ll if 64 bits. The meanings of h, l, and ll are different from their meanings in the Standard C Library. This difference is unfortunate, but this is what h, l, and ll mean in NSPR. We cannot change the specification of existing NSPR functions. Let's look at specific examples. With your patch applied, gcc generates the following warnings on the current version of pk11pars.h. ../../../../../dist/private/nss/pk11pars.h: In function ‘secmod_formatIntPair’: ../../../../../dist/private/nss/pk11pars.h:611: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘long unsigned int’ http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pk11pars.h&rev=1.25&mark=604,611#604 The code on line 611 is incorrect. ../../../../../dist/private/nss/pk11pars.h: In function ‘secmod_mkSlotString’: ../../../../../dist/private/nss/pk11pars.h:782: warning: format ‘%08lx’ expects type ‘long unsigned int’, but argument 2 has type ‘unsigned int’ ../../../../../dist/private/nss/pk11pars.h:782: warning: format ‘%d’ expects type ‘int’, but argument 5 has type ‘long unsigned int’ ../../../../../dist/private/nss/pk11pars.h:785: warning: format ‘%08lx’ expects type ‘long unsigned int’, but argument 2 has type ‘unsigned int’ http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pk11pars.h&rev=1.25&mark=757,780-781,784,785#755 In this case, the code is partially correct: * %08lx correctly matches (PRUint32)slotID on lines 782 and 785. * %d incorrectly matches timeout (an unsigned long) on line 782.

(In reply to Wan-Teh Chang from comment #7) > hiro: we can't make changes to NSPR that will break > existing code, unfortunately. Thanks for the clarification of the NSPR policy. Then I'd propose adding a macro named NSPR_USE_C_STANDARD_PRINTF_FORMAT or something like this. If the macro is defined, use GCC' format attribute in NSPR. So project can accept the GCC warnings if the project define the macro. Of course the project source code should be rewritten for the format changes though.

(In reply to Wan-Teh Chang from comment #9) Thanks for taking time. > The problem is that the existing code that causes > warnings may be correct. Easy filtering, we now the 'good ones', e.g. 'warning: int format, PRInt32 arg' does not need intervention. Many programmers seem to use Standard C arguments for PR_smprintf. This is only ok for %s/char* and %p/void* ! Another problem is mixing signed and unsigned, e.g. %d having an PRUint32 argument. The programmer should make absolutely clear, that he/she really means it ! Either by casting to PRInt32 or using a PRInt32 or using %u. I wonder if anybody ever made a code review on these issues. It's a PITA. Good luck in finding someone to fix these issues ;-)