136537 - io.c Fix for printf family format string warnings

Status: NEW

(Directory :: LDAP C SDK, defect)

Description

[Charles Shuller]

Several format string issues here, namely relying on implicit type casting to
get an expected result.

I'm a bit concerned about the first change, in line 362. 

sb->sb_sd winds up being an int, except on windows machines, where it is a
SOCKET.  According to WINE's winsock.h file, this is an unsigned int, but I
don't have a windows machine to test on, and as I recall, winsock sockets are
objects, not ints.

Comment 1

[Charles Shuller]

Attachment: fixes printf formating warnings

Comment 2

[Matthias Versen [:Matti]]

-> Ldap

Comment 3

[Srilatha Moturi]

over to dmose since it is in the c-sdk

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 78507 [details] [diff] [review]
fixes printf formating warnings

Thanks for the patch!  r=dmose; I'll check this in soon.

Comment 5

[Mark Smith [:mcs]]

Comment on attachment 78507 [details] [diff] [review]
fixes printf formating warnings

SOCKETs are unsigned ints on Windows. So we probably need to define a macro and
use it for the case around line 362, e.g.,

#ifdef _WINDOWS
#define NSLDAPI_SOCKET_T SOCKET
#else
#define NSLDAPI_SOCKET_T int
#endif
...

sprintf( ..., (NSLDAPI_SOCKET_T)sb->sb_sd, ...

And maybe another macro to choose %ld vs. %ud. Hmmm. There must be a better
way.

Does anyone know if %p is supported on all platforms? I have never used (call
me old school I guess).

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

> SOCKETs are unsigned ints on Windows. So we probably need to define a macro
> and use it for the case around line 362, e.g.,
>
> #ifdef _WINDOWS
> #define NSLDAPI_SOCKET_T SOCKET
> #else
> #define NSLDAPI_SOCKET_T int
> #endif
> ...
>
> sprintf( ..., (NSLDAPI_SOCKET_T)sb->sb_sd, ...
> 
> And maybe another macro to choose %ld vs. %ud. Hmmm. There must be a better
> way.

You're right that to be completely correct, we'd have to do something like that.
 One way to do it would be to have a 

char *socket_desc_to_string(NSLDAPI_SOCKET_T s)

function which would hide the platform-specific cruft and simply allocate and
return a string.

However, the patch as written looks ok to me, because it'll only ever have a
problem on platforms that actually return negative numbers as socket descriptors
(and maybe platforms that don't use 2's complement integer arithmetic, I guess).
 Are there any platforms that we support that actually do either of these
things?  Since this is just debugging code, and since I'm not aware of any
platforms that will trigger this, I'm inclined to accept it as is, maybe with a
comment added warning users of weird platforms... whaddya think?

> Does anyone know if %p is supported on all platforms? I have never used (call
> me old school I guess).

%p was first introduced in 4.3BSD Reno and is part of the ISO C99 standard..
Searching lxr reveals a fair number of uses of it in the Mozilla tree, including
in "fprintf" calls in NSPR, so I'll be a bit surprised if it causes us
portability problems.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Assigning bugs that I'm not actively working on back to nobody; use
SearchForThis as a search term if you want to delete all related bugmail at
once.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 78507 [details] [diff] [review]
fixes printf formating warnings

Patch has bitrotted.

$ patch --dry-run -p0 < ~/Desktop/136537.diff 
patching file io.c
Hunk #1 FAILED at 357.
Hunk #2 FAILED at 526.
Hunk #3 FAILED at 548.
3 out of 3 hunks FAILED -- saving rejects to file io.c.rej

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Untested, merely unbitrotted patch

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Mark, review ping?

cc'ing Rich and Anton, I don't know if Mark is available these days to review..

http://www.mozilla.org/about/owners.html#directory-sdk

Comment 11

[Rich Megginson]

Comment on attachment 432089 [details] [diff] [review]
Untested, merely unbitrotted patch

%ld is not unsigned int

Comment 12

[Mark Smith [:mcs]]

(In reply to comment #11)
> Comment on attachment 432089 [details] [diff] [review]
> Untested, merely unbitrotted patch
> 
> %ld is not unsigned int

Agreed.  And I don't think there is any difference between (long) and (long int).

Rich or someone else who builds this code often should probably review these changes.  The fix should be tested against the Windows (Microsoft) and Linux (gcc) compilers at least.

Comment 13

[Rich Megginson]

Comment on attachment 432089 [details] [diff] [review]
Untested, merely unbitrotted patch

I think this patch is fine if you change the %ld to %u to match the (unsigned int) format argument.

Comment 14

[Mark Banner (:standard8)]

This is actually a directory/c-sdk fix so moving to the right product/component.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 432089 [details] [diff] [review]
Untested, merely unbitrotted patch

This patch needs some tweaking as per comment 13, but I'm not sure if I have the time to work on it anymore.

aceman, perhaps you'd be interested?

Comment 16

[:aceman]

Can the problems be seen just by compiling LDAP?