Open Bug 136537 Opened 18 years ago Updated 7 years ago

io.c Fix for printf family format string warnings

Categories

(Directory :: LDAP C SDK, defect, minor)

x86
Linux
defect
Not set
minor

Tracking

(Not tracked)

People

(Reporter: CharlesShuller, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: helpwanted, Whiteboard: [patchlove][has draft patch])

Attachments

(2 obsolete files)

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.
Attached patch fixes printf formating warnings (obsolete) — Splinter Review
-> Ldap
Assignee: Matti → srilatha
Status: UNCONFIRMED → NEW
Component: Browser-General → LDAP Mail/News Integration
Ever confirmed: true
Keywords: patch, review
Product: Browser → MailNews
QA Contact: imajes-qa → yulian
over to dmose since it is in the c-sdk
Assignee: srilatha → dmose
Status: NEW → ASSIGNED
Comment on attachment 78507 [details] [diff] [review]
fixes printf formating warnings

Thanks for the patch!  r=dmose; I'll check this in soon.
Attachment #78507 - Flags: review+
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).
Attachment #78507 - Flags: review+
> 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.
QA Contact: yulian → gchan
Blocks: buildwarning
Product: MailNews → Core
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.
Assignee: dmose → nobody
Status: ASSIGNED → NEW
QA Contact: grylchan → ldap-integration
Product: Core → MailNews Core
Keywords: helpwanted
Whiteboard: [patchlove][has draft patch]
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
Attachment #78507 - Attachment is obsolete: true
Attachment #432089 - Flags: review?(mcs)
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 on attachment 432089 [details] [diff] [review]
Untested, merely unbitrotted patch

%ld is not unsigned int
(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 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.
This is actually a directory/c-sdk fix so moving to the right product/component.
Component: LDAP Integration → LDAP C SDK
Product: MailNews Core → Directory
QA Contact: ldap-integration → csdk
Version: Trunk → other
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?
Attachment #432089 - Attachment is obsolete: true
Attachment #432089 - Flags: review?(mcs)
Can the problems be seen just by compiling LDAP?
You need to log in before you can comment on or make changes to this bug.