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

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[ISHIKAWA, Chiaki]

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.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Pass correct # of arguments to format strings by means of LDAPDebug0, LDAPDebug1, etc.

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

Comment 2

[:aceman]

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.

Comment 3

[ISHIKAWA, Chiaki]

(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

Comment 4

[ISHIKAWA, Chiaki]

(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

Comment 5

[ISHIKAWA, Chiaki]

Attachment: Pass correct # of arguments to format strings by means of LDAPDebug0, LDAPDebug1, etc. (take-2)

The previous patch had issues under optimized build.

Tryserver run.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Pass correct # of arguments to format strings by means of LDAPDebug0, LDAPDebug1, etc. (take-3)

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.

Comment 7

[ISHIKAWA, Chiaki]

Attachment: Pass correct # of arguments to format strings by means of LDAPDebug0, LDAPDebug1, etc. (take-4)

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

Comment 8

[ISHIKAWA, Chiaki]

Attachment: 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.

Comment 9

[ISHIKAWA, Chiaki]

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

Comment 10

[ISHIKAWA, Chiaki]

Attachment: Pass correct # of arguments to format strings by means of LDAPDebug0, LDAPDebug1, etc. (take-5)

Fixed bitrot due to clang-format change.

Comment 11

[:aceman]

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?

Comment 12

[ISHIKAWA, Chiaki]

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

Comment 13

[ISHIKAWA, Chiaki]

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

Comment 14

[ISHIKAWA, Chiaki]

Attachment: clang-formatted: Pass correct # of arguments to format strings by means of LDAPDebug0, LDAPDebug1, etc. (take-6)

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.

Comment 15

[ISHIKAWA, Chiaki]

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

Comment 16

[ISHIKAWA, Chiaki]

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 like some manners in which formatting happens, but that is life :-)

Can you kindly take a look at this?

Comment 17

[Ben Campbell]

Attachment: 1277602-LDAPDebug-with-varargs-1.patch

These warnings began irking me enough to do some research. Turns out that variadic macros now seem to have pretty wide compiler support. This certainly wasn't the case last time I checked (OK, so that might have been about 10 years ago, but hey ;- )

So here's a patch to use variadic macros. I included the format string as one of the varargs, to sidestep the issue of a trailing comma if there are no extra parameters. The compilers still seem to vary a little on trailing-commas.

There's a couple of blocks of lint-churn in the patch too. Not sure why they didn't get picked up previously.

Try run here.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e39843807bbb903c41c792b6640fb7f0fcb64da8

@Ishikawa: I put you down for review because you've done so much work on these compiler-warning bugs, but if you prefer, just switch it to mkmelin instead :-)

Comment 18

[ISHIKAWA, Chiaki]

Finally, some is pestered enough to take a look (!) :-)

I will check this later today, thank you!

Comment 19

[ISHIKAWA, Chiaki]

Ben, the review takes a bit longer due to family business.
Thank you for your patience.

Comment 20

[Ben Campbell]

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

Thank you for your patience.

I could say the same thing to you! :-)
No rush at all - I think everything will be pretty quiet over the next couple of weeks anyway.

Comment 21

[ISHIKAWA, Chiaki]

(In reply to Ben Campbell from comment #20)

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

Thank you for your patience.

I could say the same thing to you! :-)
No rush at all - I think everything will be pretty quiet over the next couple of weeks anyway.

That was what I was beginning to think.: things get slowed down gradually. We take almost a total of a week off at the end and the start of the new year in Japan, too.

Comment 22

[Wayne Mery (:wsmwk)]

Clearing old NI from comment 16 - hopefully covered by Geoff's comment 17

Comment 23

[ISHIKAWA, Chiaki]

I will finish the review tonight. I have compiled the patch with GCC under linux and it seems OK.
It is just that I have to update another patch to make sure everything is OK locally and hopefully
submit my build job to tryserver.

Comment 24

[ISHIKAWA, Chiaki]

Comment on attachment 9194279 [details] [diff] [review]
1277602-LDAPDebug-with-varargs-1.patch

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

(Hmm. Has bugzilla gone through a face-lift? The look& feel has changed a quite a lot.)

I  found a typo in the original code.

```
diff --git a/ldap/c-sdk/libraries/libldap/os-ip.c b/ldap/c-sdk/libraries/libldap/os-ip.c
--- a/ldap/c-sdk/libraries/libldap/os-ip.c
+++ b/ldap/c-sdk/libraries/libldap/os-ip.c
@@ -284,17 +284,17 @@ static int nsldapi_os_connect_with_to(LB
   struct timeval tval;
   fd_set rset, wset;
 #    ifdef _WINDOWS
   fd_set eset;
 #    endif
 #  endif /* NSLDAPI_HAVE_POLL */
 
   LDAPDebug(LDAP_DEBUG_TRACE, "nsldapi_connect_nonblock timeout: %d (msec)\n",
-            msec, 0, 0);
+            msec);
 
 #  ifdef _WINDOWS
   ioctlsocket(sockfd, FIONBIO, &nonblock);
 #  elif defined(XP_OS2)
   ioctl(sockfd, FIONBIO, &nonblock, sizeof(nonblock));
 #  else
   flags = fcntl(sockfd, F_GETFL, 0);
   fcntl(sockfd, F_SETFL, flags | O_NONBLOCK);
@@ -330,18 +330,17 @@ static int nsldapi_os_connect_with_to(LB
   eset = rset;
 #    endif /* _WINDOWS */
 #  endif   /* NSLDAPI_HAVE_POLL */
 
   if (msec < 0 && msec != LDAP_X_IO_TIMEOUT_NO_TIMEOUT) {
     LDAPDebug(LDAP_DEBUG_TRACE,
               "Invalid timeout value detected.."
               "resetting connect timeout to default value "
-              "(LDAP_X_IO_TIMEOUT_NO_TIMEOUT\n",
-              0, 0, 0);
+              "(LDAP_X_IO_TIMEOUT_NO_TIMEOUT\n");
     msec = LDAP_X_IO_TIMEOUT_NO_TIMEOUT;
 #  ifndef NSLDAPI_HAVE_POLL
   } else {
     if (msec != 0) {
       tval.tv_sec = msec / 1000;
       tval.tv_usec = 1000 * (msec % 1000);
     } else {
       tval.tv_sec = 0;
```

additional change:
In the above part, I think we are missing the closing ")" in the string.
would you like to add the change?

---

Genuine curiosity
```
@@ -859,22 +855,20 @@ void nsldapi_dump_requests_and_responses
   ber_err_print("** Outstanding Requests:\n");
   LDAP_MUTEX_LOCK(ld, LDAP_REQ_LOCK);
   if ((lr = ld->ld_requests) == NULL) {
     ber_err_print("   Empty\n");
   }
   for (; lr != NULL; lr = lr->lr_next) {
     sprintf(msg, " * 0x%p - msgid %d,  origid %d, status %s\n", lr,
             lr->lr_msgid, lr->lr_origid,
-            (lr->lr_status == LDAP_REQST_INPROGRESS)
-                ? "InProgress"
-                : (lr->lr_status == LDAP_REQST_CHASINGREFS)
-                      ? "ChasingRefs"
-                      : (lr->lr_status == LDAP_REQST_CONNDEAD) ? "Dead"
-                                                               : "Writing");
+            (lr->lr_status == LDAP_REQST_INPROGRESS)    ? "InProgress"
+            : (lr->lr_status == LDAP_REQST_CHASINGREFS) ? "ChasingRefs"
+            : (lr->lr_status == LDAP_REQST_CONNDEAD)    ? "Dead"
+                                                        : "Writing");
     ber_err_print(msg);
     sprintf(msg, "   outstanding referrals %d, parent count %d\n",
             lr->lr_outrefcnt, lr->lr_parentcnt);
     ber_err_print(msg);
     if (lr->lr_binddn != NULL) {
       sprintf(msg, "   pending bind DN: <%s>\n", lr->lr_binddn);
       ber_err_print(msg);
     }
```
Has the code been generated by clang-format?  Indenting the
values by aligning thme at "?" operator is a great improvement of clang-format in a nested conditional expression.

I give r+, but if you can add the ")" in the said place, that will be a plus.

Comment 25

[Ben Campbell]

Attachment: 1277602-LDAPDebug-with-varargs-2.patch

This just one fixes up the missing ')'.
And also my email address, which was wrong :-)

And yes, that reformatting was down to clang-format.
I've just got a script which runs "mach clang-format" over any changed files. I have no idea how I'd go about formatting nested ternary conditions like that, so I love being able to leave that stuff up to clang-format!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ae4cc51dfd87
Fix LDAPDebug wrong-number-of-args compiler warning. r=ishikawa

Comment 27

[ISHIKAWA, Chiaki]

(In reply to Ben Campbell from comment #25)

Created attachment 9197674 [details] [diff] [review]
1277602-LDAPDebug-with-varargs-2.patch

This just one fixes up the missing ')'.

Thank you.

And also my email address, which was wrong :-)

Oh well.

And yes, that reformatting was down to clang-format.
I've just got a script which runs "mach clang-format" over any changed files. I have no idea how I'd go about formatting nested ternary conditions like that, so I love being able to leave that stuff up to clang-format!

This is amazing. clang-format is improving greatly (!).