[SERVER-9960] access violation on getCertificateName Created: 18/Jun/13 Updated: 11/Jul/16 Resolved: 19/Jun/13 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Security |
| Affects Version/s: | 2.5.0 |
| Fix Version/s: | 2.5.1 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Eric Milkie | Assignee: | Andreas Nilsson |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Environment: |
Windows |
||
| Issue Links: |
|
||||||||
| Operating System: | ALL | ||||||||
| Participants: | |||||||||
| Description |
|
It would appear that calling free() on the pointer returned by X509_NAME_online was not the correct thing to do. In debug mode, the Windows heap checker flags it as an invalid pointer to call free() on (there is a popup stating so). I then clicked on the button to dismiss the dialog and trigger a stack trace:
I expect Valgrind Memcheck might show a similar issue. |
| Comments |
| Comment by auto [ 19/Jun/13 ] | |||||||||||
|
Author: {u'username': u'agralius', u'name': u'Andreas Nilsson', u'email': u'andreas.nilsson@10gen.com'}Message: | |||||||||||
| Comment by Andreas Nilsson [ 19/Jun/13 ] | |||||||||||
|
Given that X509_NAME_oneline is deprecated and we want to use RFC2253 format anyway I rewrote getCertificateSubjectName to use X509_NAME_print_ex instead. http://codereview.10gen.com/11024035/ | |||||||||||
| Comment by Eric Milkie [ 19/Jun/13 ] | |||||||||||
|
I looked at the code now too. It's not actually that useful; it just allocates a buffer of hardcoded size 200 if you don't pass it one yourself, and then it reallocs sometimes. We'd be better off just passing our own large buffer in. The 'b' struct is allocated if you don't pass your own buffer, so the function's author has tried to make sure to deallocate the BUF_MEM at all function exit points (not an elegant job when you're in C). Basically the OPENSSL_free() is the opposite of the BUF_MEM_new(). The returned buffer was allocated via BUF_MEM_grow() and needs to be freed by the caller. | |||||||||||
| Comment by Andreas Nilsson [ 19/Jun/13 ] | |||||||||||
|
Seems other people have had the same issue: According to docs X509_NAME_oneline allocates a pointer which should be freed and this is also how OpenSSL uses the function itself when looking. However the implementation of X509_NAME_oneline ends with the lines below. To me it looks like the entire b-struct is freed on some occasions in which case the pointer p looks invalid to me.
b is a buf_mem_st struct defined as
| |||||||||||
| Comment by Tad Marshall [ 18/Jun/13 ] | |||||||||||
|
I see. Maybe we should try to get that code running earlier so you get stack traces on SSL initialization problems. I think we can set up dependencies such that the platform_init code runs before other initializers. I see that we were already in runGlobalInitializers() and we did get a stack trace from the breakpoint exception, but that gets set up in setupSignalHandlers(), which is called very early in mongoDbMain(). | |||||||||||
| Comment by Eric Milkie [ 18/Jun/13 ] | |||||||||||
|
I ran the debug version by hand (we have no debug buildslave running the Windows SSL build and the SSL test suite). | |||||||||||
| Comment by Tad Marshall [ 18/Jun/13 ] | |||||||||||
|
milkie Are we missing code to turn the popup into a logged exception? I.E. is the code in util/platform_init.cpp not catching this case? The unhandled exception is a breakpoint exception. Is this Buildbot slave running a debug version? Is the problem that this build slave doesn't have Dont_Show_UI set in the registry? Is that why there was a popup you had to click on? |