[PATCH] Fix nspr warnings with -Wstrict-prototypes

[Richard Laager]

I compiled Pidgin with -Wstrict-prototypes and got some warnings from NSPR.
Attached is a patch to fix those warnings. I haven't done any more looking into
NSPR to find other instances of this problem, so I can't say for sure if this
is it or not.

[Richard Laager (rlaager)]

Created an attachment (id=295261)
A proposed patch to fix this.

[Wan-Teh Chang (wtc-google)]

Thanks for the bug report and the patch.

Since prlink.h is a public header file and this changes two typedefs, it may break
source compatibility, so I'm afraid that we can't take this patch. Is it possible to
tell gcc to ignore strict prototype warnings about these two typedefs?

[Richard Laager (rlaager)]

(In reply to comment #2)
> Thanks for the bug report and the patch.
>
> Since prlink.h is a public header file and this changes two typedefs, it may
> break
> source compatibility, so I'm afraid that we can't take this patch.

Are these functions supposed to take arguments or not? In other words, what is the correct prototype here?

[Richard Laager (rlaager)]

I compiled Pidgin with -Wstrict-prototypes and got some warnings from NSPR.
Attached is a patch to fix those warnings. I haven't done any more looking into
NSPR to find other instances of this problem, so I can't say for sure if this
is it or not.

[Daniel Holbach (dholbach)]

To get your fix included in Ubuntu, try transforming it into a debdiff (http://wiki.ubuntu.com/PackagingGuide/Recipes/Debdiff) and submitting it for review (http://wiki.ubuntu.com/SponsorshipProcess).

Marking 'incomplete' for now.

[Daniel Holbach (dholbach)]

Setting to In Progress.

[Dan (dan-redhat-bugs)]

Description of problem:
Using nspr header gives a warning in the prlink.h file when -Wstrict-prototypes
is included in the CFLAGS. And when -Werror is used, then it is not possible to
compile at all.

Version-Release number of selected component (if applicable):
nspr-devel-4.6.7-3.fc8
but the header was not changed since Apr/06/2006, so all current Fedora and RHEL
versions are affected.

How reproducible:
100%

Steps to Reproduce:
1. run the script from attachments

Actual results:
In file included from /usr/include/nspr4/nspr.h:55,
                 from nspr-test.c:2:
/usr/include/nspr4/prlink.h:52: warning: function declaration isn’t a prototype
/usr/include/nspr4/prlink.h:209: warning: function declaration isn’t a prototype

Expected results:
No warnings

Additional info:
There was a thread in gcc mailing list about the meaning of the warning -
http://gcc.gnu.org/ml/gcc-help/2006-07/msg00033.html

[Dan (dan-redhat-bugs)]

Created attachment 309468
test case

[Dan (dan-redhat-bugs)]

Created attachment 309471
patch to remove the warnings

[Kai (kai-redhat-bugs)]

Wan-Teh, what do you think about the proposed patch?

I should probably file an upstream bug.

[SharkCZ (dan-danny)]

The same bug has been filled for Fedora/RHEL - https://bugzilla.redhat.com/show_bug.cgi?id=451616
So is there any chance for a solution?

[Kai Engert (kaie)]

If I understand correctly, the type PRFuncPtr is supposed to work with ANY function signature.

The functions that use this function-pointer type appear to operate on generic functions, which have any signature.

I conclude the proposed patch is wrong, as it would only work with functions that have a specific function signature: "empty parameter list".

Is my understanding right or wrong?

[Wan-Teh Chang (wtc-google)]

The type PRFuncPtr is like the void * pointer for function pointers.
PRFuncPtr is intended to be a universal function pointer type.
It should be cast to a specific function pointer type before calling
the function. So the function signature of PRFuncPtr doesn't matter.

I'm worried that the proposed change may break the compilation of
existing source code. Here is an example I made up:

PRBool foo(PRLibrary *lib, const char *name)
{
    /*
     * Instead of
     * PRFuncPtr func_ptr;
     * we declare the variable like this:
     */
    void (*func_ptr)();

    func_ptr = PR_FindFunctionSymbol(lib, name);
    return func_ptr != NULL;
}

I'm worried that changing the definition of PRFuncPtr
would make the compilation fail. We should test the
above code on all the platforms we support. I've tested
GCC on Linux and the code still compiles.

[Richard Laager (rlaager)]

(In reply to comment #5)
> If I understand correctly, the type PRFuncPtr is supposed to work with ANY
> function signature.
>
> The functions that use this function-pointer type appear to operate on generic
> functions, which have any signature.
>
> I conclude the proposed patch is wrong, as it would only work with functions
> that have a specific function signature: "empty parameter list".

This seems to be the case. Why not just use void * instead? This is similar to what glib does (FYI, gpointer === void * and they're using an output parameter rather than returning it): http://library.gnome.org/devel/glib/stable/glib-Dynamic-Loading-of-Modules.html#g-module-symbol

[Wan-Teh Chang (wtc-google)]

Any pointer to an object may be converted to void *.
But a function pointer is not an object pointer. So
for portability we want to use a generic function pointer
type rather than void *.

[Nelson-bolyard (nelson-bolyard)]

I recommend WONTFIX.

[SharkCZ (dan-danny)]

I think better would be first to know an opinion from some GCC or C standard people what is the best practice in such situation.

[Wan-Teh Chang (wtc-google)]

We have several options.

1. Change the implementation of the GCC -Wstrict-prototypes
to only warn about actual invocation of a function declared
without specifying the argument types. That is, the mere
declaration of a function without specifying the argument
types should not cause a warning. Only the act of calling
such a function should be warned.

This is to recognize the existence of code that defines a
generic function pointer type like this:

  typedef int (*func_ptr_t)();

or this:

  typedef void (*func_ptr_t)();

2. Do not use -Wstrict-prototypes on files that include "prlink.h".

3. Ignore the -Wstrict-prototypes warnings in "prlink.h".

4. Change "prlink.h" to suppress this warning. I think

#pragma GCC diagnostic ignored "-Wstrict-prototypes"

should work. But I don't know how to make it apply to
only certain lines in "prlink.h", and restore to the *original
setting*.

5. Change the definition of PRFuncPtr in "prlink.h". As
I explained before, NSPR needs to be backward compatible
(at both source and binary levels), which means existing
code should continue to compile against new versions of
NSPR headers. This is why I have to perform a lot of
experiments (GCC, Visual C++, Sun Studio, HP C, and IBM
C compilers) to see if changing the definition of
PRFuncPtr breaks the compilation or generates compiler
warnings on the sample code I gave in comment 6.

[Bug (bug-redhat-bugs)]

This message is a reminder that Fedora 8 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 8. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as WONTFIX if it remains open with a Fedora
'version' of '8'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version prior to Fedora 8's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that
we may not be able to fix it before Fedora 8 is end of life. If you
would still like to see this bug fixed and are able to reproduce it
against a later version of Fedora please change the 'version' of this
bug to the applicable version. If you are unable to change the version,
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

The process we are following is described here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

[Alexander Sack (asac)]

we dont need a debdiff. i can integrate patches directly. However, I dont see why we need to prepatch this. ITs properly forwarded upstream.

[Alexander Sack (asac)]

I read the upsream bug and this patch seems to be invalid.

[Bug (bug-redhat-bugs)]

Fedora 8 changed to end-of-life (EOL) status on 2009-01-07. Fedora 8 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version.

Thank you for reporting this bug and we are sorry it could not be fixed.

[Alexander Sack (asac)]

currently upstream consent seems to be wontfix. following that.

[Gk-gknw (gk-gknw)]

Folks, this bug is now 20 months old, and still no solution to this.
This is really bad practice to just ignore such bug reports since we deal here with a security-related library, and you make folks more and more accept compiler warnings which seem to be fixable from your side. When I f.e. compile a project like libcurl with NSS support these warnings in prlink.h add up to ~100 times! And disabling the warnings with -Wno-strict-prototypes is really not a solution.
BTW. I found some more places where I get similar warnings:
--- cert.h.orig 2009-07-01 01:05:24.000000000 +0200
+++ cert.h 2009-09-06 18:11:04.000000000 +0200
@@ -1610,25 +1610,25 @@
  * Returns a pointer to a static structure.
  */
 extern const CERTRevocationFlags*
-CERT_GetPKIXVerifyNistRevocationPolicy();
+CERT_GetPKIXVerifyNistRevocationPolicy(void);

 /*
  * Returns a pointer to a static structure.
  */
 extern const CERTRevocationFlags*
-CERT_GetClassicOCSPEnabledSoftFailurePolicy();
+CERT_GetClassicOCSPEnabledSoftFailurePolicy(void);

 /*
  * Returns a pointer to a static structure.
  */
 extern const CERTRevocationFlags*
-CERT_GetClassicOCSPEnabledHardFailurePolicy();
+CERT_GetClassicOCSPEnabledHardFailurePolicy(void);

 /*
  * Returns a pointer to a static structure.
  */
 extern const CERTRevocationFlags*
-CERT_GetClassicOCSPDisabledPolicy();
+CERT_GetClassicOCSPDisabledPolicy(void);

 /*
  * Verify a Cert with libpkix
@@ -1662,7 +1662,7 @@

 /* The function return PR_TRUE if cert validation should use
  * libpkix cert validation engine. */
-PRBool CERT_GetUsePKIXForValidation();
+PRBool CERT_GetUsePKIXForValidation(void);

 SEC_END_PROTOS

I think you should just apply the void fix, and as time passes by we will see if others will report new problems with the fixes (which I doubt).

[Gk-gknw (gk-gknw)]

Hi Wan-Teh Chang,
(In reply to comment #11)
> We have several options.
> 4. Change "prlink.h" to suppress this warning. I think
>
> #pragma GCC diagnostic ignored "-Wstrict-prototypes"
>
> should work. But I don't know how to make it apply to
> only certain lines in "prlink.h", and restore to the *original
> setting*.
http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
this works for me:
#pragma GCC diagnostic ignored "-Wstrict-prototypes"
#include <nspr.h>
#pragma GCC diagnostic warning "-Wstrict-prototypes"

I have tested this pragma with lowest gcc version 4.2.1 - a test with 4.0.2 shows that the pragma is not understood. Will attach a patch against prlink.h with the proper ifdefs surrounded.

[Gk-gknw (gk-gknw)]

Created an attachment (id=398986)
new patch against prlink.h to avoid warnings with pragmas

[Gk-gknw (gk-gknw)]

caveat to the patch I've posted: the pragma
#pragma GCC diagnostic warning "-Wstrict-prototypes"
also overwrites -Werror - that means if -Werror was specified from commandline then after including prlink.h the above pragma would turn it back into warnings.

[Wan-Teh Chang (wtc-google)]

(From update of attachment 398986)
Guenter,

Thanks a lot for this patch. Since prlink.h is a header
file included by many non-NSPR files, it'd be inappropriate
for us (NSPR) to turn on the -Wstrict-prototypes warning as
a result of including prlink.h. So I'm sorry that I can't
accept this patch.

It's too bad that "#pragma GCC diagnostic" doesn't have
an option for restoring to the original setting of a
warning.

[Wan-Teh Chang (wtc-google)]

(In reply to comment #12)
Guenter, could you attach your patch for cert.h to NSS
bug 515870 and ask me to review it? Thanks.

For this bug, I'll just check in <email address hidden>'s
patch. That patch requires testing with several compilers
we need to support -- we need to make sure by silencing
this GCC warning, we don't break compilation or introduce
warnings with other compilers. So it's not as trivial
as it seems.

[Gk-gknw (gk-gknw)]

Hi Wan-Teh Chang,
(In reply to comment #17)
> Guenter, could you attach your patch for cert.h to NSS
> bug 515870 and ask me to review it? Thanks.
I guess thats not needed - after I posted to this bug I checked here:
http://mxr.mozilla.org/security/source/security/nss/lib/certdb/cert.h
and from that it seems that my cert.h patch went already in before I even posted it :)
I came over these warnings with 3.12.3, but surprisingly with 3.12.4 they were gone, so I checked MXR ...

> For this bug, I'll just check in <email address hidden>'s
> patch. That patch requires testing with several compilers
> we need to support -- we need to make sure by silencing
> this GCC warning, we don't break compilation or introduce
> warnings with other compilers. So it's not as trivial
> as it seems.
Yes, the void patch seems to be the best approach to try. But you should probably attach a test sample here which others can pick up, and check on their platform if it doesnt harm.
thanks, Günter.

[Wan-Teh Chang (wtc-google)]

You're right. Someone else submitted the same patch for cert.h
in bug 491919.

[Rrelyea (rrelyea)]

> Since prlink.h is a public header file and this changes two typedefs, it
> may break source compatibility, so I'm afraid that we can't take this patch.

Sigh, the world is complicated. We just had an instance where this broke source code compatibility in NSS because prlink.h was added to a new NSS public header and the lack of ansi compatibility broke several packages.

I just did a search through the NSPR headers and found numerous uses of void in void returning functions, including such mainstays as PR_NewLock(). Is there really any doubt that all compiliers that handle NSPR will handle the voids just fine?

bob

[Wan-Teh Chang (wtc-google)]

Bob, shall we check in <email address hidden>'s patch (attachment 295261)
as I proposed in comment 17? Will that fix the problem you just reported?

[Rrelyea (rrelyea)]

>as I proposed in comment 17? Will that fix the problem you just reported?

yes, and yes. Elio, no vuvuzela's please.

[Emaldona (emaldona)]

Nagging and annoyance weapons put away. Setting the 4.8.5 target flag. Thanks.

[Wan-Teh Chang (wtc-google)]

(From update of attachment 295261)
I checked in this patch on the NSPR trunk (NSPR 4.8.6).

Checking in prlink.h;
/cvsroot/mozilla/nsprpub/pr/include/prlink.h,v <-- prlink.h
new revision: 3.17; previous revision: 3.16
done

[Elio (elio-redhat-bugs)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=410677
When we next re-base to NSPR 4.8.6 we will pick-up the fix.