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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

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

[Wan-Teh Chang]

Attachment: 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 2

[Wan-Teh Chang]

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

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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

Comment 5

[Wan-Teh Chang]

Attachment: 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 6

[Nelson Bolyard (seldom reads bugmail)]

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

r=nelson. Thanks, Wan-Teh.  This looks good to me.

Comment 7

[Julien Pierre]

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.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

> 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

[Julien Pierre]

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.

Comment 10

[Wan-Teh Chang]

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.