276314 - PK11_DestroyContext crashes if given a NULL context

Status: RESOLVED DUPLICATE of bug 276311

(NSS :: Libraries, defect)

Description

[Jason Reid]

Comment 1

[Jason Reid]

PK11_DestroyContext(NULL, PR_TRUE);

The above code causes a segmentation violation.

> gdb ./hash-tests
GNU gdb 5.3.92
Copyright 2003 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i586-suse-linux"...
(gdb) run
Starting program: /home/jmr/work/nss/tests/Hashing/hash-tests
[New Thread 16384 (LWP 3546)]
deadbeef hash tests with PK11_HashBuf
MD2= 17682ba795d60a2e740ba5f4539007a4
MD5= 4f41243847da693a4f356c0486114bc6
SHA1= f49cf6381e322b147053b74e4500af8533ac1e4c
SHA256= 2baf1f40105d9501fe319a8ec463fdf4325a2a5df445adf3f572f626253678c9
SHA384=
906d9fe92a5e0ad7e020842127f7970b7d2adff3a91106927b592cf157633d86d0be6ab78f5d398e53f705643ccbfb78
SHA512=
113a3bc783d851fc0373214b19ea7be9fa3de541ecb9fe026d52c603e8ea19c174cc0e9705f8b90d312212c0c3a6d8453ddfb3e3141409cf4bedc8ef033590b4
ERROR: PK11_HashBuf succeeded for a NULL digest
PK11_HashBuf failed for an invalide SEC_OID. (expected)
deadbeef hash tests with PK11_CreateDigestContext
MD2= 17682ba795d60a2e740ba5f4539007a4
PK11_CreateDigestContext failed to return a valid context with -1. (expected)

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 16384 (LWP 3546)]
0x400426f7 in PK11_DestroyContext (context=0x0, freeit=1) at pk11cxt.c:94
94          pk11_CloseSession(context->slot,context->session,context->ownSession);
(gdb) where
#0  0x400426f7 in PK11_DestroyContext (context=0x0, freeit=1) at pk11cxt.c:94
#1  0x08048fab in main (argc=1, argv=0xbffff1e4) at hash-tests.c:227
(gdb)

Comment 2

[Wan-Teh Chang]

Should we allow passing a null 'context' to
PK11_DestroyContext (just like it is legal to
pass a null pointer to 'free'), or should this
be considered a programming error?

My opinion is that this is a programming error.

Comment 3

[timeless]

not all free()s like null (some crash...). document it as forbidden.

Comment 4

[Wan-Teh Chang]

timeless: free(NULL) is legal and should be a no-op.
(See The C Programming Language, 2nd Ed, p. 252.)

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch

I added an assertion to detect this programming
error.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

It is a programming error, and I agree that in debug builds we should 
assert.  But in optimized builds, I think we should return rather than
crashing.  So, I'd rather see the assertion followed by a test that 
returns if NULL.

Comment 7

[timeless]

wtc: older specs for C did not specify and some c runtimes were creative.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 170410 [details] [diff] [review]
Proposed patch

See my review comments in comment 6 above.

Comment 9

[Wan-Teh Chang]

Nelson, this functions returns void.  So the only
way I can report the programming error to the caller
is a crash.

If you want the function to simply return, that means
it's not a programming error.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh,  ever since I started working on NSS 8+ years ago, it has been 
my understanding that NSS policy requires that all assertions should be 
followed by code that is executed in the non-debug case that avoids a 
crash when the asserted requirement is not met.  

The presence of the assertion clearly shows that the failure to meet 
the asserted invariant IS a programming error.  

Nevertheless, we want to avoid crashing in NSS as much as possible,
even for programming errors, in optimized builds.  That is my 
understanding of the rationale for the policy that requires asserts
to be followed by code that avoids crashes.  

In this case, if "context" is null on entry to PK11_DestroyContext, 
the existing code will crash, with or without the assert, in debug
builds and optimized builds.  So, what is the point of adding this
assert, UNLESS it is to create separation of the behavior in debug 
and optimized builds?    

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

I think we're agreed that it's a programming error to pass NULL to 
PK11_DestroyContext.  I think the only remaining question is whether
we want to make NSS "forgive" that error in optimized builds, or not.
Note that in optimized builds, NSS often forgives errors that would
cause assertion botches in debug builds.   Shall we do that here?

If so, I can attach a patch for that.
If not, we should mark this invalid.  

Wan-Teh, what is your preference?

Comment 12

[Jason Reid]

My opinion is the program should not crash. 
Passing NULL to PK11_DestroyContext may not
be legitimate but crashing a production LDAP
server over something this minor is also not
legitimate and a hassle for users. When the user
sees the core dump and produces a stack trace,
the initial blame will go to NSS even if it is 
some other groups fault.
I can the see point of crashing during DEBUG only 
builds to detect this. 

On a side note since this function returns void, why can't a 
future NSS version change the return code so the error can
be reported back to the calling function?

Comment 13

[Julien Pierre]

Jason,

Changing the return type of an existing public function means breaking binary
compatibility. We can only do this in the next major version of NSS, 4.0, for
which there is currently no plan or schedule.

Comment 14

[Wan-Teh Chang]

Nelson, please do what you think is right.

Although an assertion failure also result in
a crash, it is more informative in that it
shows the caller breaks the contract of the
API.  The reason I don't add an if statement
to prevent a crash in optimized build is
that this function returns void and a crash
is the only way to report this programming
error.

No two programmers will agree on what a
program should do when it has detected an
internal error.  So we should follow the
existing NSS policy.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

*** This bug has been marked as a duplicate of 276311 ***