Open Bug 1277602 Opened 4 years ago Updated 19 days ago

LDAP: Debug print macro LDAPDebug passes incorrect # of args to format strings and produced many WARNINGs at compile time.

Categories

(MailNews Core :: LDAP Integration, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: ishikawa, Assigned: ishikawa, NeedInfo)

Details

Attachments

(1 file, 6 obsolete files)

Hi,

I am compiling C-C TB under linux 64-bit for local testing.
For the last several weeks, I have noticed that code under ldap subdirectory has begun to produce tons of warnings regarding the mismatch of passed # of arguments and format strings.

Upon invstigation, there is a macro LDAPDebug that seems to take three arguments, on top of |level| and |format string|.
The intention of the coding was to place number 0 on the unused arguments, but recently GCC 5.0 or the addition more compiler time check has caught the passing of additional 0's to format string which lacks the format specifier for such additional 0.

I have produced four different macros:
LDAPDebug0 ... no arguments are used
LDAPDebug1 ... only the first argument is used
LDAPDebug2 ... only the first two arguments are used
LDAPDebug3 ... all the three arguments are used.

These pass the correct # of arguments to the format string so that
there is no warning.

I am attaching a patch in the next comment.
Note that there are two strange places where I needed to change the code somewhat.

1. in ldap/c-sdk/libraries/libldap/abandon.c
There is the following change.
I think |msgid| was meant to be printed in the first debug dump.
The second and third line was meant to print a pair of strange-looking numbers t me.

 int
 LDAP_CALL
 ldap_abandon( LDAP *ld, int msgid )
 {
-	LDAPDebug( LDAP_DEBUG_TRACE, "ldap_abandon %d\n", msgid, 0, 0 );
-	LDAPDebug( LDAP_DEBUG_TRACE, "4e65747363617065\n", msgid, 0, 0 );
-	LDAPDebug( LDAP_DEBUG_TRACE, "466f726576657221\n", msgid, 0, 0 );
+	LDAPDebug1( LDAP_DEBUG_TRACE, "ldap_abandon %d\n", msgid, 0, 0 );
+	LDAPDebug0( LDAP_DEBUG_TRACE, "4e65747363617065\n", 0, 0, 0 );
+	LDAPDebug0( LDAP_DEBUG_TRACE, "466f726576657221\n", 0, 0, 0 );
 
I suspect the number must have a meaning for LDAP semantics, but I am not familar to it.

2. In  ldap/c-sdk/libraries/libldap/result.c

There is incorrect code snippet which seems to be never used in TB in the current setup.
Near the line 1175 before this patch, we need to apply the following patch
to an incomplete code segment.

@@ -1182,18 +1182,19 @@ cldap_select1( LDAP *ld, struct timeval 
 
 		pollfds[0].lpoll_fd = ld->ld_sbp->sb_sd;
 		pollfds[0].lpoll_arg = ld->ld_sbp->sb_arg;
 		pollfds[0].lpoll_events = LDAP_X_POLLIN;
 		pollfds[0].lpoll_revents = 0;
 		rc = ld->ld_extpoll_fn( pollfds, 1, nsldapi_tv2ms( timeout ),
 		    ld->ld_ext_session_arg );
 	} else {
-		LDAPDebug( LDAP_DEBUG_ANY,
-		    "nsldapi_iostatus_poll: unknown I/O type %d\n",
+		LDAPDebug1( LDAP_DEBUG_ANY,
+			"nsldapi_iostatus_poll: unknown I/O type %d\n",
+			iosp->ios_type, 0, 0);
 		rc = 0; /* simulate a timeout (what else to do?) */
 	}
 
 	return( rc );
 }
 #endif /* !macintosh */
 
 
LDAPDebug macro call was not terminated, and so I presume that no platform ever uses this segment today.

Anyway, whom should I ask for review?

The number of warnings is large and actually hide other subtle warning issues in ldap code.

TIA
Assignee: nobody → ishikawa
Component: Untriaged → LDAP Integration
Product: Thunderbird → MailNews Core
Comment on attachment 8759207 [details] [diff] [review]
Pass correct # of arguments to format strings by means of LDAPDebug0, LDAPDebug1, etc.

I don't think I have noticed these warnings with GCC 5.3.

I am not sure we want to change the LDAP code so much as somehow I feel it is imported code from somewhere else. Maybe the original authors upstream should fix stuff like this.
Attachment #8759207 - Flags: review?(Pidgeot18)
(In reply to :aceman from comment #2)
> Comment on attachment 8759207 [details] [diff] [review]
> Pass correct # of arguments to format strings by means of LDAPDebug0,
> LDAPDebug1, etc.
> 
> I don't think I have noticed these warnings with GCC 5.3.
> 
> I am not sure we want to change the LDAP code so much as somehow I feel it
> is imported code from somewhere else. Maybe the original authors upstream
> should fix stuff like this.

This is imported code?
Do you know where I should report this issue.
(BTW, my gcc version is as follows.
gcc --version
gcc (Debian 5.3.1-13) 5.3.1 20160323
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Incidentally, Coverity's C front end also noticed this issue of incorrect number of
args vs the format specifiers in a format string, too.

TIA
(In reply to :aceman from comment #2)
> Comment on attachment 8759207 [details] [diff] [review]
> Pass correct # of arguments to format strings by means of LDAPDebug0,
> LDAPDebug1, etc.
> 
> I don't think I have noticed these warnings with GCC 5.3.
> 

aceman, have you possibly disabled ldap code in mozconfig file using
--disable-ldap? Then I can certainly understand that you don't see the warning.

I once disabled ldap because I thought I don't have a use for it for my local build.
However, somehow |make mozmill| started to invoke
ldap-related code (circa Oct 2014) even though I disabled ldap code, so I had to enable it again (default).

# Oct 2014. |make mozmill| runs LDAP-related test 
# although I disabled it. So took this out.
# ac_add_options --disable-ldap

TIA
The previous patch had issues under optimized build.

Tryserver run.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central
Attachment #8759207 - Attachment is obsolete: true
Attachment #8759207 - Flags: review?(Pidgeot18)
Attachment #8763294 - Flags: review?(Pidgeot18)
This is the current patch I use locally.
The latest change in the last couple of months were necessitated by some strange MS Windows compiler issues. I don't think I have changed anything to affect Windows Build on tryserver on my end, but something triggers an issue which I needed to take care of.

Anyway, this does not break the build locally and tryserver.

I now realize that the code is originally from Univ. of Michigan, but
it was picked up by Netscape to be used as ldap client library.
Again it was such a long time ago, and 
no one seems to be maintaining UPSTREAM.

Hope this helps if someone enables global format checking in compiler for local build, etc.
Attachment #8763294 - Attachment is obsolete: true
Attachment #8763294 - Flags: review?(Pidgeot18)

Updated to accommodate the clang-format changes of ldap directory.

Attachment #8901431 - Attachment is obsolete: true

ladp code needs these patches to avoid dubious sign vs. unsigned comparisons.
This patch needs to come AFTER the patch for correct # of arguments to format strings.

Attachment #9077632 - Attachment is obsolete: true

(In reply to ISHIKAWA, Chiaki from comment #8)

Created attachment 9077632 [details] [diff] [review]
Fixed the strict error check of signed vs. unsigned comparisons

ladp code needs these patches to avoid dubious sign vs. unsigned comparisons.
This patch needs to come AFTER the patch for correct # of arguments to format strings.

Oops. Wrong bugzilla. This should be posted to bug 1243121.

Fixed bitrot due to clang-format change.

Attachment #9077631 - Attachment is obsolete: true
Comment on attachment 9090157 [details] [diff] [review]
Pass correct # of arguments to format strings by means of LDAPDebug0, LDAPDebug1, etc. (take-5)

Review of attachment 9090157 [details] [diff] [review]:
-----------------------------------------------------------------

So if the new info is that there is no upstream, we could do this change.
jcranmer won't look at this. Do you need a review here, is it ready?

::: ldap/c-sdk/include/ldaplog.h
@@ +81,5 @@
>  /* SLAPD_LOGGING should not be on for WINSOCK (16-bit Windows) */
>  #  if defined(SLAPD_LOGGING)
>  #    ifdef _WIN32
>  extern int *module_ldap_debug;
>  #      define LDAPDebug(level, fmt, arg1, arg2, arg3)          \

Will this one be used by anything after this patch?

@@ +90,5 @@
>          }
> +#      define LDAPDebug0( level, fmt, arg1, arg2, arg3 )	\
> +       { \
> +		if ( *module_ldap_debug & level ) { \
> +		        slapd_log_error_proc( NULL, fmt); \

Why the added leading spaces after brackes?

::: ldap/c-sdk/libraries/libldap/abandon.c
@@ +69,5 @@
>   */
>  int LDAP_CALL ldap_abandon(LDAP *ld, int msgid) {
> +  LDAPDebug1( LDAP_DEBUG_TRACE, "ldap_abandon %d\n", msgid, 0, 0 );
> +  LDAPDebug0( LDAP_DEBUG_TRACE, "4e65747363617065\n", 0, 0, 0 );
> +  LDAPDebug0( LDAP_DEBUG_TRACE, "466f726576657221\n", 0, 0, 0 );

Do you intentionally keep the trailing zero argument for reducing the churn and visual changes?

(In reply to :aceman from comment #11)

Comment on attachment 9090157 [details] [diff] [review]
Pass correct # of arguments to format strings by means of LDAPDebug0,
LDAPDebug1, etc. (take-5)

Review of attachment 9090157 [details] [diff] [review]:

So if the new info is that there is no upstream, we could do this change.
jcranmer won't look at this. Do you need a review here, is it ready?

Yes, I say the patch is ready.
This is simply a change to shut up compiler warnings locally and eventually on tryserver.
(For now, the patches are used to test build on try server along with my other patches.
I simply throw in these LDAP-related changes before other patches.
See, for example,
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=545674bbbce29f7c6e7b92b63e277a76ac5ceb68

::: ldap/c-sdk/include/ldaplog.h
@@ +81,5 @@

/* SLAPD_LOGGING should not be on for WINSOCK (16-bit Windows) */

if defined(SLAPD_LOGGING)

ifdef _WIN32

extern int *module_ldap_debug;

define LDAPDebug(level, fmt, arg1, arg2, arg3) \

Will this one be used by anything after this patch?

I am afraid I don't understand your question here.
"Will this": what does "this" refers to?

Do you mean SLAPD_LOGGING?
I think Windows needs special treatment.
I believe I had to create Windows-specific macro,
based on earlier breakage on tryserver.
But I can not recall the details now.

@@ +90,5 @@

     }

+# define LDAPDebug0( level, fmt, arg1, arg2, arg3 ) \

  •   { \
    
  •   if ( *module_ldap_debug & level ) { \
    
  •           slapd_log_error_proc( NULL, fmt); \
    

Why the added leading spaces after brackes?

Do you refer to the space as in "{ "?
I thought it was much easier to read this way rather than "{".
But I don't know whether this matters a lot now that I realize I have to run the change through clang-format.
I wonder what the formatter would do for a code INSIDE a macro expansion.

::: ldap/c-sdk/libraries/libldap/abandon.c
@@ +69,5 @@

*/
int LDAP_CALL ldap_abandon(LDAP *ld, int msgid) {

  • LDAPDebug1( LDAP_DEBUG_TRACE, "ldap_abandon %d\n", msgid, 0, 0 );
  • LDAPDebug0( LDAP_DEBUG_TRACE, "4e65747363617065\n", 0, 0, 0 );
  • LDAPDebug0( LDAP_DEBUG_TRACE, "466f726576657221\n", 0, 0, 0 );

Do you intentionally keep the trailing zero argument for reducing the churn
and visual changes?

Exactly. If I change the number of (now unnecessary change), the visible change is too much and
my intention of reducing the compiler warnings (due to the strict change which I would think will be enabled on tryserver eventually and will result in error) without much rewriting on this code now unmaintained upstream.

I am having difficulty running |make clang-format ...| locally, and will be debugging it over the weekend.

The patch was locally clang-formatted.
It builds successfully locally and on tryserver.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8b1bbba78ca71568446d58bad2327ddb2dafda25

This should take care of my arbitrary gratuitous handling of whitespace.

Hmm, I forgot to obsolete the previous (take-5) patch, and I can't find the standalone button to make an attachment obsolete...

Aceman,

The patches here have been clang-formatted as well as one in bug 1243121.

From cursory reading, I believe all the whitespace issues that you noticed have been taken care of by clang-format.
Well, you and I may not some manners in which formatting happens, but that is life :-)

Can you kindly take a look at this?

Flags: needinfo?(acelists)
Attachment #9090157 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.