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

Greeting crash #56

Open
ThatsNotMyCode opened this issue Dec 13, 2019 · 5 comments
Open

Greeting crash #56

ThatsNotMyCode opened this issue Dec 13, 2019 · 5 comments

Comments

@ThatsNotMyCode
Copy link

ThatsNotMyCode commented Dec 13, 2019

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" (

return errGreeting
).

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

  • zmq4 should handle the greeting as specified in the ZMTP protocol
  • zmq4 should not give panic errors

Unfortunately I lack Go skills to fix this bug.

@sbinet
Copy link
Contributor

sbinet commented Dec 13, 2019

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
Copy link
Author

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 added a commit to sbinet-alice/zmq4 that referenced this issue Jan 20, 2020
sbinet added a commit to sbinet-alice/zmq4 that referenced this issue Jan 20, 2020
sbinet added a commit to sbinet-alice/zmq4 that referenced this issue Jan 20, 2020
@sbinet
Copy link
Contributor

sbinet commented Jan 20, 2020

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
Copy link
Contributor

IoTMOD commented Jan 20, 2020

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:

zmq4/socket.go

Lines 194 to 198 in 018f24d

zconn, err := Open(conn, sck.sec, sck.typ, sck.id, true, sck.scheduleRmConn)
if err != nil {
panic(err)
// return xerrors.Errorf("zmq4: could not open a ZMTP connection: %w", err)
}

I'll open a PR, if wanted

@sbinet
Copy link
Contributor

sbinet commented Jan 21, 2020

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)

sbinet added a commit to sbinet-alice/zmq4 that referenced this issue Jan 21, 2020
sbinet added a commit to sbinet-alice/zmq4 that referenced this issue Jan 21, 2020
This CL also makes sure we exercize invalid connection attempts and
still end up with a valid, non-corrupted, socket state.

Updates go-zeromq#56.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants