The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in 3.12

Status

NSS
Libraries
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.06 KB, patch
Julien Pierre
: review+
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 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.
(Assignee)

Comment 1

11 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)
(Assignee)

Comment 2

11 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
#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.
(Assignee)

Comment 4

11 years ago
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
(Assignee)

Comment 5

11 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

11 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

11 years ago
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+
(Assignee)

Comment 10

11 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
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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
You need to log in before you can comment on or make changes to this bug.