The C Programming Language, 2nd. Ed., Sec. A7.3.2 says: ... trailing arguments beyond the explicitly typed parameters suffer default argument promotion ... If 0 (an int constant) is passed as an unnamed argument, it won't be converted to a null pointer. So the function will extract the wrong value if the sizes of pointer and int are different, such as on 64-bit platforms.
Created attachment 212148 [details] [diff] [review] Proposed patch There are only two NSS functions that follow the convention of using a null pointer to terminate an array of pointers: extern CERTName *CERT_CreateName(CERTRDN *rdn, ...); extern CERTRDN *CERT_CreateRDN(PRArenaPool *arena, CERTAVA *avas, ...); They are only called three times: name = CERT_CreateName(NULL); rdn = CERT_CreateRDN(name->arena, ava, 0); trdn = CERT_CreateRDN(arena, 0); The first call is fine. The second call must be fixed. The third call (in which the 0 is for a named function parameter) should be fixed.
Comment on attachment 212148 [details] [diff] [review] Proposed patch Note that the solution of using NULL only works in C code. In C++ code we'd have to use (void *)0 or (CERTAVA *)0 in these instances, because the NULL macro is typically defined like this: #ifdef __cplusplus #define NULL 0 #else #define NULL ((void *)0) #endif
Attachment #212148 - Flags: superreview? → superreview?(nelson)
Comment on attachment 212148 [details] [diff] [review] Proposed patch Wan-Teh, is it possible that the sense of that ifdef in comment 2 is reversed? and that NULL is cast as a void * for C++, and not for c? I believe there are platforms on which NULL is defined as 0 for the c language. I once saw a long debate on this subject, which concluded that 0 (not a cast of zero) is the right choice for C, and I have believed that ever since. As I understand it, you want to tell the compiler that this zero is to be passed as a pointer, not as an integer. In that case, I think it best to explicity cast it to be a void*, so that it will be a pointer, even on platforms that define NULL to be merely 0.
Nelson: The sense of that ifdef is not reversed. The definition in comment 2 was copied from MSVC 6.0. Here is the Linux definition: #if defined(__cplusplus) #define NULL 0 #else #define NULL ((void *)0) #endif I'll attach a new patch with explicit pointer casts. Which do you like the best? (void *)0 (CERTAVA *)0 (CERTAVA *)NULL
Status: NEW → ASSIGNED
Created attachment 212155 [details] [diff] [review] Proposed patch v2 I decided to use (CERTAVA *)0 in the first instance because the neighboring code uses 0 as null pointers as in C++ code. In the second instance, I simply use NULL, because that is the second argument, which is typed (CERTAVA *avas).
Comment on attachment 212155 [details] [diff] [review] Proposed patch v2 r=nelson. Thanks, Wan-Teh. This looks good to me.
Attachment #212155 - Flags: superreview?(nelson) → review+
The second change in the patch passes NULL . Are we sure it's always declared as (void)* on all compilers ? If so, then r+ . Otherwise, passing something like (XXX *)0 or even (XXX*) NULL would be better.
> Are we sure it's always declared as (void)* on all compilers ? It shouldn't matter in the second case. The type of the second formal parameter to that function should ensure that the compiler converts the argument to a pointer for this invocation. The first case is different because the formal parameter is "...", which has no type, so the cast is needed there.
Comment on attachment 212155 [details] [diff] [review] Proposed patch v2 Nelson, Thanks, that clears it. I didn't realize in the second part of the patch that the argument was not the vararg one.
Attachment #212155 - Flags: review?(julien.pierre.bugs) → review+
I checked in the patch on the NSS trunk (3.12). Checking in alg1485.c; /cvsroot/mozilla/security/nss/lib/certdb/alg1485.c,v <-- alg1485.c new revision: 1.25; previous revision: 1.24 done Checking in secname.c; /cvsroot/mozilla/security/nss/lib/certdb/secname.c,v <-- secname.c new revision: 1.19; previous revision: 1.18 done Since this bug was discovered by code inspection, it's not clear if it should be backported to NSS_3_11_BRANCH. It only affects 64-bit platforms, and it only affects the CERT_AsciiToName function.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
You need to log in before you can comment on or make changes to this bug.