Last Comment Bug 476126 - CERT_AsciiToName fails when AVAs in an RDN are separated by '+'
: CERT_AsciiToName fails when AVAs in an RDN are separated by '+'
Status: RESOLVED FIXED
SUN_MUST_HAVE
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.0
: All All
: P1 major (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-29 23:51 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2009-02-08 17:17 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 for NSS Trunk (1.72 KB, patch)
2009-01-29 23:51 PST, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
Details | Diff | Splinter Review
supplemental patch (3.03 KB, patch)
2009-02-05 12:26 PST, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2009-01-29 23:51:07 PST
Created attachment 359711 [details] [diff] [review]
Patch v1 for NSS Trunk

When an RDN has multiple AVAs, they are separated in the ASCII string form
by '+' rather than by ',' or ';'.  CERT_NameToAscii generates that form 
of separator correctly, but CERT_AsciiToName fails when it encounters a '+'
separator.  This means that commands like certutil, which take a DN in ASCII
form on the command line, fail if the DN uses '+' to separate any AVAs.
CERT_AsciiToName is presently unable to create RDNs with multiple AVAs.
There's an existing function for adding AVAs to RDNs, and it is unused.
It's amazing that we've not run into this problem before now.

The attached patch seems to fix it.  Julien, please review it.
Sun CR 6799382.
Comment 1 Julien Pierre 2009-02-04 19:45:06 PST
Comment on attachment 359711 [details] [diff] [review]
Patch v1 for NSS Trunk

I think this patch is OK, but in ParseRFC1485Name, there should probably be a comment about the

if (bp[-1] != '+')

test. It appears to be reading undefined memory. It doesn't really, because bp has been incremented due to the successful call to CERT_ParseRFC1485AVA . 
It would be more elegant to add an argument to that function to signify that the RDN is done when + is encountered, so the caller doesn't have to parse backwards. That's a stylistic matter so I won't block the patch for that, as long as a comment is added.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-02-05 11:25:47 PST
Checking in alg1485.c; new revision: 1.29; previous revision: 1.28

I will add a comment, as Julien suggested.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2009-02-05 12:26:32 PST
Created attachment 360777 [details] [diff] [review]
supplemental patch

This patch adds an explicit definition for the behavior of the AVA parsing
function, and makes the function behave according to that definition.
It also makes the function static.  It is basically a clean up.  
Julien, please review.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-02-08 17:17:19 PST
Checking in certdb/alg1485.c; new revision: 1.30; previous revision: 1.29

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