[PATCH] Fix nspr warnings with -Wstrict-prototypes

Bug #180179 reported by Richard Laager
4
Affects Status Importance Assigned to Milestone
NSPR
Fix Released
Medium
nspr (Fedora)
Fix Released
Low
nspr (Ubuntu)
Low
Unassigned
pidgin (Ubuntu)
Undecided
Unassigned

Bug Description

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.

Revision history for this message
In , Richard Laager (rlaager) wrote :

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

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

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?

Revision history for this message
In , Richard Laager (rlaager) wrote :

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

Revision history for this message
Richard Laager (rlaager) wrote :

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.

Changed in nspr:
status: Unknown → In Progress
Revision history for this message
Richard Laager (rlaager) wrote :
Revision history for this message
Daniel Holbach (dholbach) wrote :

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.

Changed in nspr:
status: New → Incomplete
Revision history for this message
Daniel Holbach (dholbach) wrote :

Setting to In Progress.

Changed in nspr:
status: Incomplete → In Progress
Revision history for this message
In , Dan (dan-redhat-bugs) wrote :

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

Revision history for this message
In , Dan (dan-redhat-bugs) wrote :

Created attachment 309468
test case

Revision history for this message
In , Dan (dan-redhat-bugs) wrote :

Created attachment 309471
patch to remove the warnings

Revision history for this message
In , Kai (kai-redhat-bugs) wrote :

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

I should probably file an upstream bug.

Revision history for this message
In , SharkCZ (dan-danny) wrote :

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?

Changed in nspr:
status: Unknown → Confirmed
Revision history for this message
In , Kai Engert (kaie) wrote :

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?

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

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.

Revision history for this message
In , Richard Laager (rlaager) wrote :

(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

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

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

Revision history for this message
In , Nelson-bolyard (nelson-bolyard) wrote :

I recommend WONTFIX.

Revision history for this message
In , SharkCZ (dan-danny) wrote :

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.

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

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.

Revision history for this message
In , Bug (bug-redhat-bugs) wrote :

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

Revision history for this message
Alexander Sack (asac) wrote :

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

Changed in nspr:
importance: Undecided → Low
status: In Progress → Triaged
Revision history for this message
Alexander Sack (asac) wrote :

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

Revision history for this message
In , Bug (bug-redhat-bugs) wrote :

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.

Changed in nspr:
status: Confirmed → Won't Fix
Jonathan Davies (jpds)
Changed in pidgin (Ubuntu):
status: New → Invalid
Revision history for this message
Alexander Sack (asac) wrote :

currently upstream consent seems to be wontfix. following that.

Changed in nspr (Ubuntu):
status: Triaged → Won't Fix
Revision history for this message
In , Gk-gknw (gk-gknw) wrote :

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

Revision history for this message
In , Gk-gknw (gk-gknw) wrote :

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.

Revision history for this message
In , Gk-gknw (gk-gknw) wrote :

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

Revision history for this message
In , Gk-gknw (gk-gknw) wrote :

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.

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

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

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

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

Revision history for this message
In , Gk-gknw (gk-gknw) wrote :

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.

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

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

Revision history for this message
In , Rrelyea (rrelyea) wrote :

> 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

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

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?

Revision history for this message
In , Rrelyea (rrelyea) wrote :

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

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

Revision history for this message
In , Emaldona (emaldona) wrote :

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

Revision history for this message
In , Wan-Teh Chang (wtc-google) wrote :

(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

Changed in nspr:
status: In Progress → Fix Released
Revision history for this message
In , Elio (elio-redhat-bugs) wrote :

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.

Changed in nspr:
importance: Unknown → Medium
Changed in nspr (Fedora):
importance: Unknown → Low
status: Won't Fix → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.