Can't pass 0 as an unnamed null pointer argument to CERT_CreateRDN



13 years ago
13 years ago


(Reporter: wtc, Assigned: wtc)


Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



13 years ago
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.

Comment 1

13 years ago
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.
Attachment #212148 - Flags: superreview?
Attachment #212148 - Flags: review?(julien.pierre.bugs)

Comment 2

13 years ago
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
#define NULL    ((void *)0)
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.

Comment 4

13 years ago

The sense of that ifdef is not reversed.  The definition
in comment 2 was copied from MSVC 6.0.  Here is the Linux

#if defined(__cplusplus)
#define NULL 0
#define NULL ((void *)0)

I'll attach a new patch with explicit pointer casts.

Which do you like the best?

(void *)0

Comment 5

13 years ago
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).
Attachment #212148 - Attachment is obsolete: true
Attachment #212155 - Flags: superreview?(nelson)
Attachment #212155 - Flags: review?(julien.pierre.bugs)
Attachment #212148 - Flags: superreview?(nelson)
Attachment #212148 - Flags: review?(julien.pierre.bugs)
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+

Comment 7

13 years ago
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 9

13 years ago
Comment on attachment 212155 [details] [diff] [review]
Proposed patch v2


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+

Comment 10

13 years ago
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
Checking in secname.c;
/cvsroot/mozilla/security/nss/lib/certdb/secname.c,v  <--  secname.c
new revision: 1.19; previous revision: 1.18

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