go-zeromq / zmq4

[WIP] Pure-Go implementation of ZeroMQ-4
BSD 3-Clause "New" or "Revised" License
342 stars 57 forks source link

Greeting crash #56

Open ThatsNotMyCode opened 4 years ago

ThatsNotMyCode commented 4 years ago

If I use NetMQ (https://github.com/zeromq/netmq) to connect to a socket hosted by zmq4, the zmq4 host crashes with "zmq4: invalid greeting received" (https://github.com/go-zeromq/zmq4/blob/78ce94b745426940eb8d9849125a2b9d99a496cd/protocol.go#L115).

It seems that NetMQ does follow the negotiation protocol correctly (https://rfc.zeromq.org/spec:23/ZMTP/#version-negotiation), but zmq4 does not...

Unfortunately I lack Go skills to fix this bug.

sbinet commented 4 years ago

do you have the panic traceback? (before investing time in implementing "version negotiation", I'd prefer to be sure this is actually the issue at hand :P)

ThatsNotMyCode commented 4 years ago

image

Hi sbinet,

I only have this stacktrace. And I can provide a minimal example project that causes the crash. Example code: https://filebin.net/okurjnjpohukq2oo Compiled for Linux: https://filebin.net/hldy9oyn0ips4aan (connects to tcp://localhost:5556)

sbinet commented 4 years ago

I have updated zmq4 to display the version announced by the greeting. could you update the issue with the version you received? that would help in devising the amount of work to fix this.

thanks.

IoTMOD commented 4 years ago

Hi @sbinet, regarding this issue there is another point. For example with a nmap localhost -p <zmq-port> we'll get a panic too. Couldn't we replace the panic(err) with a continue? This way we'll ignore if a connection couldn't be opened, but we wouldn't break active connections due to the panic: https://github.com/go-zeromq/zmq4/blob/018f24d5c483816bc2f6a768cab199b58ceda27d/socket.go#L194-L198

I'll open a PR, if wanted

sbinet commented 4 years ago

looks like the panic is an old "debug catch all" of mine. continue-ing seems fine. (perhaps we should at some point bubble up that error w/o breaking the accept-loop. but that probably can be addressed in another PR)