277394 - Patch for prprf.h to enable gcc's printf argument checking

Status: RESOLVED WONTFIX

(NSPR :: NSPR, enhancement)

Description

[Tim Ruehsen]

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:

Comment 1

[Tim Ruehsen]

Attachment: cvs diff

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

Comment 2

[Wan-Teh Chang]

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 3

[Wan-Teh Chang]

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.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(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?

Comment 7

[Wan-Teh Chang]

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

Comment 8

[Tim Ruehsen]

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.

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 11

[Tim Ruehsen]

(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 ;-)