Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visual Studio 2012 x64 Unhandled Exception Error #2344

Closed
danieljbarber opened this issue Feb 18, 2017 · 27 comments
Closed

Visual Studio 2012 x64 Unhandled Exception Error #2344

danieljbarber opened this issue Feb 18, 2017 · 27 comments

Comments

@danieljbarber
Copy link

With the latest stable release 4.2.1, I'm having an issue where the creation and deletion of a socket causes a crash in Visual Studio 2012 64 bit builds. I've tried building the library from the provided msvc projects and using CMake and get the same result. This does not happen in later versions of VS that I have tested (2015), and also does not occur in 32 bit builds in VS2012. I've pasted sample code below, the program will crash with an unhandled exception error inside of the libzmq library, in a thread the socket or context creates when using a map on line 77 of select.cpp. I've included sample code that demonstrates this. This occurs when running in both debug and release under x64.

#include <zmq.h>

int main()
{
    zmq_msg_t msg;
    void* context = zmq_ctx_new();
    void* socket = zmq_socket(context, ZMQ_SUB);

    if(socket)
    {
        zmq_close(socket);
    }

    if(context)
    {
        zmq_ctx_destroy(context);
    }

    return 0;
}
@bluca
Copy link
Member

bluca commented Feb 18, 2017

It's all working fine on Linux, OSX, BSDs and even in the Windows CI which uses VS2015 I think: https://ci.appveyor.com/project/zeromq/libzmq/build/build-1038/job/cmao6jfotef1j6qa

So I assume it's a problem with the VS2012 configuration. Sorry but I really know nothing about Visual Studio, hopefully a Windows developer can help.

If you know VS and can find the issue please do send a PR.

@evoskuil
Copy link
Contributor

I'd give it a look but I'm AFK for another week. I wouldn't rule out an issue in the code, especially given that CMake and VS projects give same result. There is a lot of conditionality in the sources.

@bluca
Copy link
Member

bluca commented Feb 18, 2017

Thanks!

@bjovke
Copy link
Contributor

bjovke commented Apr 10, 2017

@danieljbarber I've compiled x64 version on VS 2012, ran your program and indeed there's an exception.
The program stops because of stack overflow (1MB default in VS 2012).
When stack is set to 4MB everything works fine: Project -> xxx Properties -> Configuration properties -> Linker -> System -> Stack Reserve Size -> 4194304.
We'll see why is this happening but you now have a workaround.

@bjovke
Copy link
Contributor

bjovke commented Apr 12, 2017

@danieljbarber @bluca @evoskuil
Figured it out. In select.cpp, line 77, there's this code:

  • family_entry_t& family_entry = family_entries [family];

family_entries is std::map<u_short, family_entry_t> and family_entry_t contains three fd_set s.
In VS 2012 x64 build one fd_set is 131080 bytes: 16384 * 8 + 8 = 131080. In the end, family_entry_t is 393280 bytes.

That's one part of the issue, second one, the main culprit, is:
std::map::operator[] is used on line 77, here's the VS 2012's implementation:

	mapped_type& operator[](const key_type& _Keyval)
		{	// find element matching _Keyval or insert with default mapped
		iterator _Where = this->lower_bound(_Keyval);
		if (_Where == this->end()
			|| this->_Getcomp()(_Keyval, this->_Key(_Where._Mynode())))
			_Where = this->insert(_Where,
				pair<key_type, mapped_type>(
					_Keyval,
					mapped_type()));
		return (_Where->second);
		}

It allocates from stack two family_entry_t s, which is 786560 bytes!

It succeeds two times and third call of this socket.cpp line is reached in zmq_ctx_destroy(context); in your program. At that moment 395216 bytes of stack have been spent and std::map::operator[] finally pushes it over the edge (of course, if stack size is default 1 MB).

For the same program, VS 2015 x64 build, std::map::operator[] at this place consumes only 456 bytes of stack!. std::map::operator[] is obviously different and looks like this:

	template<class _Keyty,
		class... _Mappedty>
		_Pairib _Try_emplace(_Keyty&& _Keyval,
			_Mappedty&&... _Mapval)
		{	// fail if _Keyval present, else emplace
		iterator _Where = _Mybase::lower_bound(_Keyval);
		if (_Where == _Mybase::end()
			|| _DEBUG_LT_PRED(_Mybase::_Getcomp(),
				_Keyval, _Mybase::_Key(_Where._Mynode())))
			return (_Pairib(
				_Mybase::emplace_hint(_Where,
					piecewise_construct,
					_STD forward_as_tuple(
						_STD forward<_Keyty>(_Keyval)),
					_STD forward_as_tuple(
						_STD forward<_Mappedty>(_Mapval)...)),
				true));
		else
			return (_Pairib(_Where, false));
		}

So the main problem is in std::map implementation in VS 2012, combined with large object size stored in it.

This is happening only on x64 build because it has more data structures padding than 32-bit version and stack consumption for 32-bit version is a little bit lower than 1 MB, so there's no program crash.

On Linux fd_set is a bit field, much smaller than on Windows. On Windows it is an array of 4-byte SOCKETs, but what makes it worse is that all elements are padded to 8 bytes. That means that on Linux fd_set size is 16384 bits = 2048 bytes and on Windows it is 16384 * 8 +8 = 131080 bytes.

Possible solutions:

  • Set larger stack on Windows when using VS 2012.
  • Rewrite libzmq code to avoid this situation. I think that this would be overkill. Increasing stack size for any application is a normal thing to do during development.

@bluca
Copy link
Member

bluca commented Apr 12, 2017

Great analysis. Could you please send a PR to add a note to the INSTALL file document in the Windows section to suggest increasing the stack? This way it's documented and it's easier for users.

@evoskuil
Copy link
Contributor

Crazy. I run VS2013 - very curious to know if this is still an issue, since I use std::map for other stuff.

@bjovke
Copy link
Contributor

bjovke commented Apr 13, 2017

@evoskuil Here's a test which you can run:

#include <map>

int main()
{
  typedef struct {
    uint64_t array[1000];
  } test_a;

  std::map<int, test_a> test_map;
  test_a t=test_map[1];

  return 0;
}

Start debugging, add esp/rsp register to Watch (32/64 bit).
Step into test_a t=test_map[1]; and observe how stack pointer changes. If it's little above 16000 bytes then there's an issue in implementation.

@bjovke
Copy link
Contributor

bjovke commented Apr 13, 2017

@bluca @evoskuil @danieljbarber Sure, I'll add PR. But I don't know should I put that on Windows it is recommended to always increase stack size from default, just for VS 2012 or only when stack overflow is noticed? Also I haven't tested VS 2013.
Personally I think that increase should be mandatory for VS 2012 and as needed for other versions. Programmer should always figure out if he needs more stack or not.
But, for the sake of consistency on different platforms maybe it's good to mandatory have larger stack on Windows?

@bluca
Copy link
Member

bluca commented Apr 13, 2017

Perhaps rather than indicating it's mandatory, simply stating that with that version of VS there might be these kind of problems, and this is the right workaround

@bjovke
Copy link
Contributor

bjovke commented Apr 13, 2017

@bluca @evoskuil Ok. What about VS 2013? It would be nice if someone can check it who has it installed? Otherwise I need to install it.

@evoskuil
Copy link
Contributor

I'll give it a look.

@evoskuil
Copy link
Contributor

evoskuil commented Apr 14, 2017

RSP changes (decreases) 8 bytes upon stepping into test_a t=test_map[1] in VS2013.

@bjovke
Copy link
Contributor

bjovke commented Apr 14, 2017

@evoskuil Step one more time. When you step into std::map::operator[] cursor will be on opening bracket of function. Write down rsp there, step one more time with F10 and write down rsp again. Then you subtract those two values.

@evoskuil
Copy link
Contributor

okay, 128:

0000006972EFD6F8
0000006972EFD5D0

@bjovke
Copy link
Contributor

bjovke commented Apr 14, 2017

296 bytes. That means std::map is fixed in VS 2013. Great, thank you!
I'll make a PR.

@bluca
Copy link
Member

bluca commented Apr 14, 2017

Fixed by #2532

@bluca bluca closed this as completed Apr 14, 2017
@kachanovskiy
Copy link
Contributor

This issue is also seen with VS2010 Win64, where std::map is presumably having the same issue, though the code is slightly different.

While I am not quite sure the following workaround deserves a PR (due to introducing an extra dependency), I will put it here for those who may be hitting this too (for example, being in the same situation as me: VS2010 Win64, no chance to increase stack size since what I do is a DLL and setting stack size is only done at process level, which is out of my control):

WORKAROUND:

boost::container::map seems to be a drop-in replacement for std::map, therefore, if you already use Boost, or at least have it installed and are allowed to use it in your project, just use something like this:

diff --git a/src/select.hpp b/src/select.hpp
index e76ebcaa..dce8c7f5 100644
--- a/src/select.hpp
+++ b/src/select.hpp
@@ -36,7 +36,8 @@
 
 #include <stddef.h>
 #include <vector>
-#include <map>
+// #include <map>
+#include "boost/container/map.hpp"
 
 #if defined ZMQ_HAVE_WINDOWS
 #elif defined ZMQ_HAVE_OPENVMS
@@ -128,7 +129,8 @@ class select_t : public poller_base_t
                               struct timeval &tv_);
 
 #if defined ZMQ_HAVE_WINDOWS
-    typedef std::map<u_short, family_entry_t> family_entries_t;
+    // typedef std::map<u_short, family_entry_t> family_entries_t;
+    typedef boost::container::map<u_short, family_entry_t> family_entries_t;
 
     struct wsa_events_t
     { 

Cheers.

Sergey.

@bluca
Copy link
Member

bluca commented Feb 14, 2018

Sorry I'm not faimiliar at all with Windows - why is increasing the stack size not an option?

@kachanovskiy
Copy link
Contributor

Stack is an attribute of a code execution, i.e. running process or a light-weight process, also known as thread. What I am developing is a DLL, and DLL is not a thread or a process, and won't be unless it is dynamically loaded and called by a running process - but then it will be running in context of a calling process, hence will have the stack of calling process. it is identical to what shared library on Unix/Linux looks like.

similar topic discussed here:
https://software.intel.com/en-us/forums/intel-visual-fortran-compiler-for-windows/topic/600697

while there IS a way to set stack size for a process (using "The Microsoft COFF Binary File Editor (EDITBIN.EXE)" from Visual Studio), in my case this is highly discouraged way of solving stack overflow, because a) this may be rendering host application (that calls my application) ustable and unsupported, and b) I cannot recommend this to my customers since they do not have VS, and neither can I re-distribute editbin.exe.

Cheers.

Sergey.

@bluca
Copy link
Member

bluca commented Feb 14, 2018

IIRC this specific problem happens in the I/O thread, which we control - can the stack be increased just for that thread?

@sigiesec
Copy link
Member

The stack size can be specified in the call to _beginthreadex: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex

@bluca
Copy link
Member

bluca commented Feb 14, 2018

Do you think it's a good idea to do it?

@kachanovskiy
Copy link
Contributor

In theory, this is plausible. However, the downside of it is potentially running into stack overflow again if the value you pass will not be sufficient.

If what is proposed in INSTALL is believed to be a good value - I can send a PR with the fix.

@bjovke
Copy link
Contributor

bjovke commented Feb 14, 2018 via email

@bjovke
Copy link
Contributor

bjovke commented Feb 14, 2018 via email

@sigiesec
Copy link
Member

Another option were to build with POLLER=poll, which is, however, broken in 4.2.3, but should be fine in the current master's HEAD (we have only recently added CI tests for that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants