Open Bug 136537 Opened 19 years ago Updated 8 years ago
.c Fix for printf family format string warnings
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.
over to dmose since it is in the c-sdk
Assignee: srilatha → 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 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).
> 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.
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
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
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?
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.