Last Comment Bug 327529 - Can't pass 0 as an unnamed null pointer argument to CERT_CreateRDN
: Can't pass 0 as an unnamed null pointer argument to CERT_CreateRDN
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All All
: -- normal (vote)
: 3.12
Assigned To: Wan-Teh Chang
: Jason Reid
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-16 13:45 PST by Wan-Teh Chang
Modified: 2006-02-20 15:11 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (1.06 KB, patch)
2006-02-16 14:03 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Proposed patch v2 (1.06 KB, patch)
2006-02-16 15:25 PST, Wan-Teh Chang
julien.pierre: review+
nelson: review+
Details | Diff | Splinter Review

Description Wan-Teh Chang 2006-02-16 13:45:39 PST
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 2006-02-16 14:03:03 PST
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 2 Wan-Teh Chang 2006-02-16 14:15:45 PST
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) 2006-02-16 15:06:49 PST
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 2006-02-16 15:15:51 PST
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 2006-02-16 15:25:01 PST
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 6 Nelson Bolyard (seldom reads bugmail) 2006-02-16 16:04:14 PST
Comment on attachment 212155 [details] [diff] [review]
Proposed patch v2

r=nelson. Thanks, Wan-Teh.  This looks good to me.
Comment 7 Julien Pierre 2006-02-17 18:51:17 PST
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) 2006-02-17 19:04:17 PST
> 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 2006-02-17 19:24:02 PST
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 2006-02-20 15:11:20 PST
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.

Note You need to log in before you can comment on or make changes to this bug.